[web] Move styling from FlutterViewEmbedder to StyleManager #47489

mdebbar · 2023-10-30T22:02:06Z

Move default style values from FlutterViewEmbedder to StyleManager.
Move DOM tag names from FlutterViewEmbedder to DomManager.
Deduplicate style sheet creation code and put it in StyleManager.

ditman

Small comment to unify the global_styles.dart and the StyleManager into one big single, interrelated blob, but consider all my comments as severity "nitpick" at most!

lib/web_ui/lib/src/engine/text/ruler.dart

lib/web_ui/lib/src/engine/view_embedder/dom_manager.dart

lib/web_ui/lib/src/engine/global_styles.dart

lib/web_ui/lib/src/engine/text/canvas_paragraph.dart

lib/web_ui/test/html/shaders/shader_mask_golden_test.dart

ditman

I don't have much left to say here! A couple of minor questions about some small implementation details, but other than that: 🚀

ditman · 2023-11-09T20:52:26Z

lib/web_ui/lib/src/engine/text/paragraph.dart

-  return fontWeightIndexToCss(fontWeightIndex: fontWeight.index);
 }

 String fontWeightIndexToCss({int fontWeightIndex = 3}) {


Is fontWeightIndexToCss still needed outside of the FontWeightExtension? Should the implementation of this method be the actual implementation of toCssString() there? Or could this method be made a private method on the FontWeightExtension?

Also, can this reimplemented as:

case 3: return 'normal'; case 6: return 'bold'; default: return '${(fontWeightIndex + 1) * 100}';

(This might be in a super-critical part of the code so we might not want to do math on it)

This method is being used in text_editing.dart where the framework sends us the font weight index in a platform message.

Does this happen in many places? Shouldn't we convert the platform message to a proper ui.FontWeight with something like: ui.FontWeight.values[index]?

lib/web_ui/lib/src/engine/text/paragraph.dart

ditman · 2023-11-09T20:59:42Z

lib/web_ui/lib/src/engine/text/ruler.dart

+  // Font family.
  result.write(canonicalizeFontFamily(fontFamily));

  return result.toString();


Having a StringBuffer in this method looks weird to me, why not:

return '$cssFontStyle $cssFontWeight $cssFontSize ${canonicalizeFontFamily(fontFamily)}';

So there's no need to instantiate a string buffer?

Yep, makes sense!

ditman · 2023-11-09T21:01:08Z

lib/web_ui/lib/src/engine/view_embedder/style_manager.dart

-    $cssSelectorPrefix flt-scene-host {
+    $cssSelectorPrefix ${DomManager.sceneHostTagName} {


auto-submit · 2023-11-09T22:16:14Z

auto label is removed for flutter/engine/47489, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

ditman

(Still leaving my approval, but for fontSize I think we should prefer int to num (or double?)

ditman · 2023-11-10T00:05:59Z

lib/web_ui/lib/src/engine/text/ruler.dart

-  final StringBuffer result = StringBuffer();
+  final String cssFontStyle = fontStyle?.toCssString() ?? StyleManager.defaultFontStyle;
+  final String cssFontWeight = fontWeight?.toCssString() ?? StyleManager.defaultFontWeight;
+  final num cssFontSize = fontSize?.floor() ?? StyleManager.defaultFontSize;


It probably doesn't matter much, but:

Suggested change

final num cssFontSize = fontSize?.floor() ?? StyleManager.defaultFontSize;

final int cssFontSize = fontSize?.floor() ?? StyleManager.defaultFontSize;

Good idea. I did it slightly differently though because the rest of the code expects font size to be a double.

final int cssFontSize = (fontSize ?? StyleManager.defaultFontSize).floor();

(and kept StyleManager.defaultFontSize a double).

ditman · 2023-11-10T00:06:19Z

lib/web_ui/lib/src/engine/text/ruler.dart

-  result.write(canonicalizeFontFamily(fontFamily));
-
-  return result.toString();
+  return '$cssFontStyle $cssFontWeight ${cssFontSize}px $cssFontFamily';


ditman · 2023-11-10T00:07:39Z

lib/web_ui/lib/src/engine/view_embedder/style_manager.dart

+class StyleManager {
+  static const String defaultFontStyle = 'normal';
+  static const String defaultFontWeight = 'normal';
+  static const double defaultFontSize = 14;


Suggested change

static const double defaultFontSize = 14;

static const int defaultFontSize = 14;

…flutter/engine#47489)

…138237) flutter/engine@a5eab0d...77349dc 2023-11-10 [email protected] [Android] Bump robolectric version to support unit testing on Android 34 (flutter/engine#47768) 2023-11-10 [email protected] Roll Skia from ae6df7264c6e to 5df579e87784 (1 revision) (flutter/engine#47910) 2023-11-10 [email protected] [web] Move styling from FlutterViewEmbedder to StyleManager (flutter/engine#47489) 2023-11-10 [email protected] Roll Skia from e23e328584a1 to ae6df7264c6e (1 revision) (flutter/engine#47907) 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

[web] Move styling from FlutterViewEmbedder to StyleManager

abf1653

mdebbar requested a review from ditman October 30, 2023 22:02

github-actions bot added the platform-web Code specifically for the web engine label Oct 30, 2023

mdebbar added 2 commits October 31, 2023 10:32

Merge branch 'main' into style_manager

d60a37f

Merge branch 'main' into style_manager

09bce52

ditman approved these changes Nov 9, 2023

View reviewed changes

mdebbar added 9 commits November 9, 2023 11:24

Merge branch 'main' into style_manager

fd1937e

null safety improvements

c8c358d

drop the "k"

e6d0ce1

pass dpr

829648b

global_styles.dart => style_manager.dart

51b2b1e

StyleManager => style_manager.dart

3087905

style_manager_test.dart

9b10ac1

update imports

40d0926

Merge branch 'main' into style_manager

7408b36

ditman approved these changes Nov 9, 2023

View reviewed changes

no StringBuffer in buildCssFontString

136f9d5

mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2023

ditman approved these changes Nov 10, 2023

View reviewed changes

mdebbar added 2 commits November 10, 2023 10:25

int cssFontSize

2fee68f

Merge branch 'main' into style_manager

4d7f883

mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2023

auto-submit bot merged commit 5e3fdec into flutter:main Nov 10, 2023

mdebbar deleted the style_manager branch November 10, 2023 16:11

engine-flutter-autoroll mentioned this pull request Nov 10, 2023

Roll Flutter Engine from a5eab0d281fe to 77349dc8e27b (4 revisions) flutter/flutter#138237

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 10, 2023

5e3fdecb2 [web] Move styling from FlutterViewEmbedder to StyleManager (…

9883a14

…flutter/engine#47489)

		$cssSelectorPrefix flt-scene-host {
		$cssSelectorPrefix ${DomManager.sceneHostTagName} {

	final num cssFontSize = fontSize?.floor() ?? StyleManager.defaultFontSize;
	final int cssFontSize = fontSize?.floor() ?? StyleManager.defaultFontSize;

	static const double defaultFontSize = 14;
	static const int defaultFontSize = 14;

[web] Move styling from FlutterViewEmbedder to StyleManager #47489

[web] Move styling from FlutterViewEmbedder to StyleManager #47489

Uh oh!

Conversation

mdebbar commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

ditman Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Nov 9, 2023

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mdebbar commented Oct 30, 2023 •

edited

Loading

ditman Nov 9, 2023 •

edited

Loading