Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] Fix default line-height issue for Firefox #13960

Merged
merged 1 commit into from
Nov 21, 2019
Merged

[web] Fix default line-height issue for Firefox #13960

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions lib/web_ui/lib/src/engine/dom_renderer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: these could be convenient reusable getters in the browser_detection.dart library.

// TODO(butterfly): use more efficient CSS selectors; descendant selectors
// are slow. More info:
//
// https://csswizardry.com/2011/09/writing-efficient-css-selectors/

// This undoes browser's default layout attributes for paragraphs. We
// compute paragraph layout ourselves.
sheet.insertRule('''
flt-ruler-host p, flt-scene p {
margin: 0;
}''', sheet.cssRules.length);
if (isFirefox) {
// For firefox set line-height, otherwise textx at same font-size will
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: "textx"

// measure differently in ruler.
sheet.insertRule(
'flt-ruler-host p, flt-scene p '
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, we are doing this here: #32043 🎉

'{ margin: 0; line-height: 100%;}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it break things if we add this to other browsers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra CSS matchers slow down browser style recalculation. We should avoid them whenever possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are already adding the same CSS matcher to other browser, just without line-height.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you're right. Doesn't matter in this case then.

sheet.cssRules.length);
} else {
sheet.insertRule(
'flt-ruler-host p, flt-scene p '
'{ margin: 0; }',
sheet.cssRules.length);
}

// This undoes browser's default painting and layout attributes of range
// input, which is used in semantics.
Expand All @@ -248,15 +258,15 @@ flt-semantics input[type=range] {
left: 0;
}''', sheet.cssRules.length);

if (browserEngine == BrowserEngine.webkit) {
if (isWebKit) {
sheet.insertRule(
'flt-semantics input[type=range]::-webkit-slider-thumb {'
' -webkit-appearance: none;'
'}',
sheet.cssRules.length);
}

if (browserEngine == BrowserEngine.firefox) {
if (isFirefox) {
sheet.insertRule(
'input::-moz-selection {'
' background-color: transparent;'
Expand Down Expand Up @@ -292,7 +302,7 @@ flt-semantics [contentEditable="true"] {

// By default on iOS, Safari would highlight the element that's being tapped
// on using gray background. This CSS rule disables that.
if (browserEngine == BrowserEngine.webkit) {
if (isWebKit) {
sheet.insertRule('''
flt-glass-pane * {
-webkit-tap-highlight-color: transparent;
Expand Down Expand Up @@ -393,8 +403,7 @@ flt-glass-pane * {
// is 1.0.
window.debugOverrideDevicePixelRatio(1.0);

if (html.window.visualViewport == null &&
browserEngine == BrowserEngine.webkit) {
if (html.window.visualViewport == null && isWebKit) {
// Safari sometimes gives us bogus innerWidth/innerHeight values when the
// page loads. When it changes the values to correct ones it does not
// notify of the change via `onResize`. As a workaround, we setup a
Expand Down