Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[web] Apply global styles before inserting the DOM element #48027

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Nov 14, 2023

The current way we are doing global styles (using CSSStyleSheet) forces us to insert the DOM element into the document before we attach any styles to the element. That restriction goes away if we append the styles as TextNodes into the <style> element.

Now with the movement towards DomManager, I would like to be able to create the entire DOM tree of the Flutter View (including all styles) before we insert it into the document.

Part of flutter/flutter#134443

@mdebbar mdebbar requested a review from ditman November 14, 2023 17:49
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Nov 14, 2023
font: $defaultCssFont;
}
''', sheet.cssRules.length);
styleElement.appendText(
Copy link
Member

@ditman ditman Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a violation of the CSP of the page. Have you tried to run an app with this change with the following CSP?

script-src 'nonce-YOUR_NONCE_VALUE' 'wasm-unsafe-eval';
font-src https://fonts.gstatic.com;
style-src 'nonce-YOUR_NONCE_VALUE';

(The CSP can be set with just a meta-tag MDN, it needs to be a one-liner I think)

((Remember to set the nonce values in your flutter.js initialization, as described here))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh, didn't think about that. Thanks for bringing it up. I'll try it and see what happens.

Copy link
Contributor Author

@mdebbar mdebbar Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be working fine in a sample app that I tried (I had to add nonce in many places to get it working 😜).

@ditman were you concerned that Element.appendText(...) is not allowed with CSP? Now that I think about it, how is it different from the insertRule API? They both take an arbitrary string.

Copy link
Member

@ditman ditman Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that insertRule is inserting the rule in CSSOM (so there's no possibility of XSS, AFAIK), but insertText is appending stuff in the DOM, so I thought that either TrustedTypes or CSP wouldn't like it! (I guess it's more like textContent than innerHTML 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's more like textContent than innerHTML 😅

Exactly. If you look at appendText in dom.dart, it's basically creating a Text Node and appending it. (and then, I assume, the CSS parser will parse it the same way it parses the string passed to insertRule).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just passing by but I believe XSS is possible in css too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cedvdb I this case I think this is all right, because the stylesheet that is being created and injected is signed with flutter's nonce.

@mdebbar mdebbar requested a review from ditman November 15, 2023 15:55
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Mouad checked that this wouldn't hurt CSP/TT!

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 15, 2023
@auto-submit auto-submit bot merged commit b61160a into flutter:main Nov 15, 2023
24 checks passed
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 16, 2023
…138529)

flutter/engine@3cfcdeb...8aff9c1

2023-11-16 [email protected] Roll Dart SDK from cc6acfd7d57c to 5cccc24d127f (1 revision) (flutter/engine#48109)
2023-11-16 [email protected] Roll Skia from 5d6bdbf69dea to add865f891c8 (1 revision) (flutter/engine#48108)
2023-11-16 [email protected] Roll Dart SDK from 65819963fb17 to cc6acfd7d57c (5 revisions) (flutter/engine#48100)
2023-11-16 [email protected] Make `fml/status_or.h` compatible with `.clang_tidy`. (flutter/engine#48002)
2023-11-16 [email protected] [Impeller] Gate Vulkan selection on API 29 (flutter/engine#48089)
2023-11-16 [email protected] [macOS] Clean up allocations in menu plugin test (flutter/engine#48093)
2023-11-16 [email protected] Re-land "Make `fml/...` compatible with `.clang_tidy` (flutter/engine#48030)
2023-11-15 [email protected] [ios] introduce weak_nsobject (flutter/engine#47947)
2023-11-15 [email protected] Roll Skia from e954d1a1972c to 5d6bdbf69dea (2 revisions) (flutter/engine#48094)
2023-11-15 [email protected] [Impeller] add async command submission for blit pass. (flutter/engine#48040)
2023-11-15 [email protected] Make `lib/ui/compositing/...` compatible with `.clang_tidy`. (flutter/engine#48001)
2023-11-15 [email protected] Remove the linux fuchsia v1 build. (flutter/engine#48085)
2023-11-15 [email protected] [web] Apply global styles before inserting the DOM element (flutter/engine#48027)
2023-11-15 [email protected] Roll Skia from b23074a79bda to e954d1a1972c (7 revisions) (flutter/engine#48092)
2023-11-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make `lib/ui/{text|window}/...` compatible with `.clang_tidy`." (flutter/engine#48083)
2023-11-15 [email protected] [ios]fix ios 16 auto correction highlight showing on top left corner (flutter/engine#47279)
2023-11-15 [email protected] Roll Skia from c42226314a4f to b23074a79bda (3 revisions) (flutter/engine#48081)
2023-11-15 [email protected] Make `lib/ui/{text|window}/...` compatible with `.clang_tidy`. (flutter/engine#48000)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@mdebbar mdebbar deleted the style_sheet branch November 16, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants