[web] Add support for textScaleFactor #29028

mdebbar · 2021-10-05T20:23:54Z

Read the computed font size of the <html> element and use that as a guide to figure out the textScaleFactor. The default value should be 16.

$$textScaleFactor = (font size of `<html>`) / 16.0$$

I was able to try this on Chrome and Firefox. I couldn't find a way to change the text scale factor in Safari.

Fixes flutter/flutter#52061

ditman · 2021-10-08T16:38:36Z

I couldn't find a way to change the text scale factor in Safari.

@mdebbar I think you can set it in Safari -> Settings -> Advanced -> Never use font sizes smaller than...

yjbanov

LGTM with nitpicks.

yjbanov · 2021-10-14T00:15:42Z

lib/web_ui/lib/src/engine/platform_dispatcher.dart

+  // Using JavaScript's `window.parseFloat` here because it can parse values
+  // like "20px", while Dart's `double.tryParse` fails.
+  final num? result =
+      js_util.callMethod(html.window, 'parseFloat', <Object>[source]) as num?;


Is it possible to use package:js for this (see canvaskit_api.dart)?

(applies throughout the file)

I was able to use package:js for parseFloat. But I'm not sure how to do that for others like:

js_util.callMethod(element, 'computedStyleMap', <Object?>[])

Is this possible to convert to package:js?

yjbanov · 2021-10-14T00:26:21Z

lib/web_ui/lib/src/engine/platform_dispatcher.dart

+  if (js_util.hasProperty(element, 'computedStyleMap')) {
+    // Use the newer `computedStyleMap` API available on some browsers.
+    final dynamic computedStyleMap =
+        js_util.callMethod(element, 'computedStyleMap', <Object?>[]);


Have you considered using https://api.flutter.dev/flutter/dart-html/Window/getComputedStyleMap.html?

I just tried it. It throws an exception saying that window.getComputedStyleMap is undefined. I also looked online and couldn't find this property on window. The only one that exists is window.getComputedStyle which I'm already using below through dart:html.

yjbanov · 2021-10-14T00:27:54Z

lib/web_ui/lib/src/engine/platform_dispatcher.dart

+    // Use the newer `computedStyleMap` API available on some browsers.
+    final dynamic computedStyleMap =
+        js_util.callMethod(element, 'computedStyleMap', <Object?>[]);
+    if (computedStyleMap is Object) {


Is is Object a null check? If so, why not != null?

It is a null check PLUS a type check/inference so when I pass it to js_util.callMethod I don't need to do as Object.

Strange that != null doesn't promote to Object automatically. Generics allow you to declare non-nullness by doing <T extends Object>, so I'd expect != null to also be equivalent to is Object. But oh well.

yjbanov · 2021-10-14T00:28:07Z

lib/web_ui/lib/src/engine/platform_dispatcher.dart

+    if (computedStyleMap is Object) {
+      final dynamic fontSizeObject =
+          js_util.callMethod(computedStyleMap, 'get', <Object?>['font-size']);
+      if (fontSizeObject is Object) {


rileyporter · 2021-10-15T00:06:29Z

Several of the Dart js_util functions now have a generic return type which will affect this. It looks like the change to add the generic return type landed and rolled into Flutter. So far, there have been no analyzer errors, so I don't anticipate needing to roll this change back. You will probably get implicit_dynamic_function analyzer errors on this code now that you can fix by adding explicit types. Examples:

Explicit casts are no longer necessary and they will cause implicit dynamic casts, so they should be removed when the type can be inferred (i.e. from the variable type declaration) e.g.:

final num? result =
    js_util.callMethod(html.window, 'parseFloat', <Object>[source]); // remove the `as num?`

Declare the generic the type explicitly when the type cannot be inferred, or where the type is declared as dynamic, e.g:

js_util.callMethod<void>(o, 'foo', []);

final dynamic fontSizeObject =
         js_util.callMethod<dynamic>(computedStyleMap, 'get', <Object?>['font-size']);

Let me know if any of that is confusing or if you need any help fixing this code for the analyzer. Hopefully it will be a smooth change and make the js_util code easier to read in the future.

mdebbar · 2021-10-15T15:00:56Z

@mdebbar I think you can set it in Safari -> Settings -> Advanced -> Never use font sizes smaller than...

@ditman thanks for the tip! That worked and I was able to confirm that we read textScaleFactor correctly on Safari too.

mdebbar · 2021-10-15T15:41:46Z

@rileyporter thanks for the details!

I just pulled the latest, and I can see this commit which rolls your dart-sdk change into the engine. But in my IDE (VS Code), I still don't see the analyzer errors that you are talking about.

The PR has also passed all the checks (including the analyzer) without me doing any changes.

rileyporter · 2021-10-15T17:37:09Z

Sorry, my change did end up getting reverted. There is a dart linter bug that my change exposes which triggers for a couple cases in internal google code with false positives. The linter bug has been fixed, but it hasn't been rolled internally yet. I'm disabling the lints in the google code temporarily so I can re-land my change to hopefully keep other folks from writing more Flutter code using js_util that will need to be migrated.

I downloaded a patch of this PR to verify my change does cause analyzer issues with the js_util usage:

dart analyze --fatal-infos
Analyzing web_ui...                    4.7s

  error • lib/src/engine/platform_dispatcher.dart:1099:17 • Missing type arguments for generic function 'callMethod<T>'. Try adding an explicit type, or remove
          implicit-dynamic from your analysis options file. • implicit_dynamic_function
  error • lib/src/engine/platform_dispatcher.dart:1102:19 • Missing type arguments for generic function 'callMethod<T>'. Try adding an explicit type, or remove
          implicit-dynamic from your analysis options file. • implicit_dynamic_function
  error • lib/src/engine/platform_dispatcher.dart:1104:28 • Missing type arguments for generic function 'getProperty<T>'. Try adding an explicit type, or remove
          implicit-dynamic from your analysis options file. • implicit_dynamic_function

3 issues found.

Can you add // ignore: implicit_dynamic_function to these three places in this PR to keep it from breaking when I re-land?

Sorry for the inconvenience, I'll remove those comments and add explicit types in this PR after the generic types land.

[web] Add support for textScaleFactor

7cb6389

mdebbar added the platform-web Code specifically for the web engine label Oct 5, 2021

mdebbar requested a review from yjbanov October 5, 2021 20:23

google-cla bot added the cla: yes label Oct 5, 2021

mdebbar requested a review from ditman October 7, 2021 13:59

mdebbar added 2 commits October 7, 2021 13:14

Merge branch 'master' into text_scale_factor

15ffe21

Merge branch 'master' into text_scale_factor

5c4689f

mdebbar added 2 commits October 11, 2021 11:33

Merge branch 'master' into text_scale_factor

882228c

Merge branch 'master' into text_scale_factor

bd1cc67

yjbanov approved these changes Oct 14, 2021

View reviewed changes

mdebbar added 3 commits October 15, 2021 10:08

use package:js instead of js_util

f144ea9

Merge branch 'master' into text_scale_factor

fba505e

tests for parseFloat

c58f1de

mdebbar requested a review from yjbanov October 15, 2021 15:59

mdebbar added 2 commits October 18, 2021 10:20

Merge branch 'master' into text_scale_factor

9fb0d25

ignores

46bb77b

mdebbar mentioned this pull request Oct 18, 2021

[web] Implement onTextScaleFactorChanged in flutter web flutter/flutter#91327

Closed

yjbanov approved these changes Oct 18, 2021

View reviewed changes

mdebbar merged commit 8758e37 into flutter:master Oct 19, 2021

engine-flutter-autoroll mentioned this pull request Oct 19, 2021

Roll Engine from 9830def3a494 to ed57c8a58c32 (3 revisions) flutter/flutter#92117

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 19, 2021

8758e37 [web] Add support for textScaleFactor (flutter/engine#29028)

8e34cfa

engine-flutter-autoroll mentioned this pull request Oct 19, 2021

Roll Engine from 9830def3a494 to 112fb84b5dc1 (4 revisions) flutter/flutter#92126

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 19, 2021

8758e37 [web] Add support for textScaleFactor (flutter/engine#29028)

ea4be50

kylinchen pushed a commit to XianyuTech/engine that referenced this pull request Oct 22, 2021

[web] Add support for textScaleFactor (flutter#29028)

d80157f

mdebbar deleted the text_scale_factor branch January 17, 2023 18:34

[web] Add support for textScaleFactor #29028

[web] Add support for textScaleFactor #29028

Uh oh!

Conversation

mdebbar commented Oct 5, 2021

Uh oh!

ditman commented Oct 8, 2021

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

yjbanov Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

mdebbar Oct 15, 2021 • 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!

yjbanov Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

mdebbar Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

yjbanov Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

mdebbar Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

yjbanov Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

yjbanov Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

rileyporter commented Oct 15, 2021

Uh oh!

mdebbar commented Oct 15, 2021

Uh oh!

mdebbar commented Oct 15, 2021

Uh oh!

rileyporter commented Oct 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mdebbar Oct 15, 2021 •

edited

Loading