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.

Sign up for GitHub

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

Jump to bottom

[web] Remove unused function parameter #41700

Merged
merged 2 commits into from
May 3, 2023
Merged

[web] Remove unused function parameter #41700

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07cbcd6
[web] Remove unused function parameter
mdebbar Feb 27, 2023
0b74a88
put back cssSelectorPrefix
mdebbar May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/web_ui/lib/src/engine/embedder.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,6 @@ class FlutterViewEmbedder {
shadowRoot.appendChild(shadowRootStyleElement);
applyGlobalCssRulesToSheet(
shadowRootStyleElement,
hasAutofillOverlay: browserHasAutofillOverlay(),
defaultCssFont: defaultCssFont,
);

Expand Down
38 changes: 18 additions & 20 deletions lib/web_ui/lib/src/engine/global_styles.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import 'text_editing/text_editing.dart';
// Applies the required global CSS to an incoming [DomCSSStyleSheet] `sheet`.
void applyGlobalCssRulesToSheet(
DomHTMLStyleElement styleElement, {
required bool hasAutofillOverlay,
String cssSelectorPrefix = '',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, turns out the cssSelectorPrefix param is used by the new text editing host node:

engine/lib/web_ui/lib/src/engine/embedder.dart

Line 437 in d2dce25

cssSelectorPrefix: FlutterViewEmbedder.flutterViewTagName,

I have 2 questions:

  1. Is that necessary because it's outside of the shadow dom?
  2. Since the styles are scoped to flutter-view, should we attach it somewhere global so that it applies to all flutter-views?

cc @htoor3 @ditman

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it more necessary now that it's out of the shadowDOM? I thought one of the big gains we got from the shadowDOM was that the styles don't leak and affect things outside of the app's context. Now that inputs are outside of the shadowDOM, the selector prefix makes sure we're only applying these styles to flutter app inputs.

Option 2 sounds like a good idea. Also having multiple flutter-views means multiple <style> elements get created unnecessarily currently. Having one style sheet at the top level would probably be a cleaner approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't it more necessary now that it's out of the shadowDOM? I thought one of the big gains we got from the shadowDOM was that the styles don't leak and affect things outside of the app's context. Now that inputs are outside of the shadowDOM, the selector prefix makes sure we're only applying these styles to flutter app inputs.

Alright, cool, that's what I thought too. So you are saying "yes" to my first question :)

Option 2 sounds like a good idea. Also having multiple flutter-views means multiple <style> elements get created unnecessarily currently. Having one style sheet at the top level would probably be a cleaner approach.

Yep. I'll leave it as is for now. I'm happy to revisit and do this when we start figuring things out for multi-view.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh why did I read your comment as saying it was "unnecessary" 😂

Yeah we're in agreement. Sounds good to me on revisiting for later!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I put the cssSelectorPrefix parameter back. Now the PR is only removing the hasAutofillOverlay parameter.

required String defaultCssFont,
}) {
// TODO(web): use more efficient CSS selectors; descendant selectors are slow.
Expand All @@ -24,7 +22,7 @@ void applyGlobalCssRulesToSheet(
//
// Fixes #115216 by ensuring that our parameters only affect the flt-scene-host children.
sheet.insertRule('''
$cssSelectorPrefix flt-scene-host {
flt-scene-host {
color: red;
font: $defaultCssFont;
}
Expand All @@ -34,7 +32,7 @@ void applyGlobalCssRulesToSheet(
// on using gray background. This CSS rule disables that.
if (isSafari) {
sheet.insertRule('''
$cssSelectorPrefix * {
* {
-webkit-tap-highlight-color: transparent;
}
''', sheet.cssRules.length);
Expand All @@ -46,8 +44,8 @@ void applyGlobalCssRulesToSheet(
//
// - See: https://github.com/flutter/flutter/issues/44803
sheet.insertRule('''
$cssSelectorPrefix flt-paragraph,
$cssSelectorPrefix flt-span {
flt-paragraph,
flt-span {
line-height: 100%;
}
''', sheet.cssRules.length);
Expand All @@ -56,7 +54,7 @@ void applyGlobalCssRulesToSheet(
// This undoes browser's default painting and layout attributes of range
// input, which is used in semantics.
sheet.insertRule('''
$cssSelectorPrefix flt-semantics input[type=range] {
flt-semantics input[type=range] {
appearance: none;
-webkit-appearance: none;
width: 100%;
Expand All @@ -71,7 +69,7 @@ void applyGlobalCssRulesToSheet(

if (isSafari) {
sheet.insertRule('''
$cssSelectorPrefix flt-semantics input[type=range]::-webkit-slider-thumb {
flt-semantics input[type=range]::-webkit-slider-thumb {
-webkit-appearance: none;
}
''', sheet.cssRules.length);
Expand All @@ -80,27 +78,27 @@ void applyGlobalCssRulesToSheet(
// The invisible semantic text field may have a visible cursor and selection
// highlight. The following 2 CSS rules force everything to be transparent.
sheet.insertRule('''
$cssSelectorPrefix input::selection {
input::selection {
background-color: transparent;
}
''', sheet.cssRules.length);
sheet.insertRule('''
$cssSelectorPrefix textarea::selection {
textarea::selection {
background-color: transparent;
}
''', sheet.cssRules.length);

sheet.insertRule('''
$cssSelectorPrefix flt-semantics input,
$cssSelectorPrefix flt-semantics textarea,
$cssSelectorPrefix flt-semantics [contentEditable="true"] {
flt-semantics input,
flt-semantics textarea,
flt-semantics [contentEditable="true"] {
caret-color: transparent;
}
''', sheet.cssRules.length);

// Hide placeholder text
sheet.insertRule('''
$cssSelectorPrefix .flt-text-editing::placeholder {
.flt-text-editing::placeholder {
opacity: 0;
}
''', sheet.cssRules.length);
Expand All @@ -110,10 +108,10 @@ void applyGlobalCssRulesToSheet(
// See: https://github.com/flutter/flutter/issues/118337.
if (browserHasAutofillOverlay()) {
sheet.insertRule('''
$cssSelectorPrefix .transparentTextEditing:-webkit-autofill,
$cssSelectorPrefix .transparentTextEditing:-webkit-autofill:hover,
$cssSelectorPrefix .transparentTextEditing:-webkit-autofill:focus,
$cssSelectorPrefix .transparentTextEditing:-webkit-autofill:active {
.transparentTextEditing:-webkit-autofill,
.transparentTextEditing:-webkit-autofill:hover,
.transparentTextEditing:-webkit-autofill:focus,
.transparentTextEditing:-webkit-autofill:active {
opacity: 0 !important;
}
''', sheet.cssRules.length);
Expand All @@ -129,7 +127,7 @@ void applyGlobalCssRulesToSheet(
// the ::-ms-reveal pseudo-selector).
try {
sheet.insertRule('''
$cssSelectorPrefix input::-ms-reveal {
input::-ms-reveal {
display: none;
}
''', sheet.cssRules.length);
Expand All @@ -140,7 +138,7 @@ void applyGlobalCssRulesToSheet(
// Add a fake rule if our code failed because we're under testing
assert(() {
sheet.insertRule('''
$cssSelectorPrefix input.fallback-for-fakey-browser-in-ci {
input.fallback-for-fakey-browser-in-ci {
display: none;
}
''', sheet.cssRules.length);
Expand Down
1 change: 0 additions & 1 deletion lib/web_ui/test/engine/global_styles_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ void testMain() {
domDocument.body!.append(styleElement);
applyGlobalCssRulesToSheet(
styleElement,
hasAutofillOverlay: browserHasAutofillOverlay(),
defaultCssFont: _kDefaultCssFont,
);
});
Expand Down