[web] Fix default line-height issue for Firefox #13960
Conversation
| margin: 0; | ||
| }''', sheet.cssRules.length); | ||
| if (isFirefox) { | ||
| // For firefox set line-height, otherwise textx at same font-size will |
| // measure differently in ruler. | ||
| sheet.insertRule( | ||
| 'flt-ruler-host p, flt-scene p ' | ||
| '{ margin: 0; line-height: 100%;}', |
There was a problem hiding this comment.
Does it break things if we add this to other browsers?
There was a problem hiding this comment.
Extra CSS matchers slow down browser style recalculation. We should avoid them whenever possible.
There was a problem hiding this comment.
We are already adding the same CSS matcher to other browser, just without line-height.
There was a problem hiding this comment.
Ah, you're right. Doesn't matter in this case then.
| @@ -220,18 +220,28 @@ class DomRenderer { | |||
| _styleElement = html.StyleElement(); | |||
| html.document.head.append(_styleElement); | |||
| final html.CssStyleSheet sheet = _styleElement.sheet; | |||
|
|
|||
| final bool isWebKit = browserEngine == BrowserEngine.webkit; | |||
| final bool isFirefox = browserEngine == BrowserEngine.firefox; | |||
There was a problem hiding this comment.
optional: these could be convenient reusable getters in the browser_detection.dart library.
| // measure differently in ruler. | ||
| sheet.insertRule( | ||
| 'flt-ruler-host p, flt-scene p ' | ||
| '{ margin: 0; line-height: 100%;}', |
There was a problem hiding this comment.
Extra CSS matchers slow down browser style recalculation. We should avoid them whenever possible.
| // For firefox set line-height, otherwise textx at same font-size will | ||
| // measure differently in ruler. | ||
| sheet.insertRule( | ||
| 'flt-ruler-host p, flt-scene p ' |
There was a problem hiding this comment.
I'm wondering now if we are seeing expensive style recalculation because of these descendant matchers. Something we should try at some point is use a custom tag for paragraphs and use a simple tag matcher instead.
There was a problem hiding this comment.
Great point. We could also get away with simply adding a class name to all the p tags we use for text measurement and rendering. I'll file an issue so we don't forget.
There was a problem hiding this comment.
Finally, we are doing this here: #32043 🎉
[email protected]:flutter/engine.git/compare/96fc607b8a54...7e68097 git log 96fc607..7e68097 --no-merges --oneline 2019-11-21 [email protected] [web] Fix line-height for Firefox (flutter/engine#13960) 2019-11-21 [email protected] [web] Allow users to enable canvas text measurement (flutter/engine#13929) 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] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Fixes flutter/flutter#44803