stylo: site issue: large fonts on zdf.de

VERIFIED FIXED in Firefox 56

Status

()

VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: darkspirit, Assigned: emilio)

Tracking

(Blocks: 1 bug, {nightly-community})

56 Branch
mozilla56
x86_64
Linux
nightly-community
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

Created attachment 8880884 [details]
2017-06-23_zdf.de_1.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170623115718

Steps to reproduce:

See images.
* fonts are too large
* If you click on Download, then X, fonts get even bigger
* video is larger than its parent element, so video controls are not visible
* html5 video tag poster + play button on it is missing.
(Reporter)

Comment 1

2 years ago
Created attachment 8880885 [details]
2017-06-23_zdf.de_2.png
(Reporter)

Updated

2 years ago
Blocks: 1375906
Has STR: --- → yes
Keywords: nightly-community
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
(Reporter)

Updated

2 years ago
Summary: stylo: site issue: zdf.de → stylo: site issue: large fonts and oversized video on zdf.de
(Assignee)

Comment 2

2 years ago
This is going to be a rem units issue, potentially similar to bug 1374062.
(Assignee)

Comment 3

2 years ago
Gah, got it.
Assignee: nobody → emilio+bugs
(Assignee)

Updated

2 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 4

2 years ago
Thanks for all the reports btw, they're awesome!
(Assignee)

Comment 5

2 years ago
I'm actually not sure how we've had this so wrong for so long... I bet this is causing some tests to fail.
Comment hidden (mozreview-request)
(Reporter)

Comment 7

2 years ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> Thanks for all the reports btw, they're awesome!

Thanks for coding Stylo! (in Rust <3)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8880952 [details]
Bug 1375930: Fix rem computation on the root element.

https://reviewboard.mozilla.org/r/152322/#review157464

::: commit-message-6e8f2:3
(Diff revision 2)
> +I'll be really pissed off if this doesn't make any new test pass and I have to
> +write one...
> +
> +I don't actually know how could we get this so wrong for so long...

This doesn't belong in a commit message.

::: servo/components/style/values/specified/length.rs:154
(Diff revision 2)
> +                // 	 When specified on the font-size property of the root
> +                // 	 element, the rem units refer to the property’s initial
> +                // 	 value.

Replace tabs with spaces.
Attachment #8880952 - Flags: review?(cam) → review+
I'm a bit curious why this bug only appears when restyling, and not on the initial style.  Do you know why?

Comment 12

2 years ago
mozreview-review
Comment on attachment 8881015 [details]
Bug 1375930: Test that rem units for the root font-size aren't resolved against the root font size.

https://reviewboard.mozilla.org/r/152384/#review157466

r=me but this test feels like it's veering into the realm of testing for a browser specific bug, so might be more appropriate in layout/reftests/bugs/ or somewhere.

::: layout/reftests/w3c-css/submitted/values3/rem-root-font-size-restyle-1.html:22
(Diff revision 1)
> +<div></div>
> +<script>
> +  document.documentElement.offsetTop;
> +  // Force a style recalc.
> +  document.documentElement.style.color = "green";
> +  document.documentElement.offsetTop;

This flush isn't needed.  (The reftest harness will wait for the styles to be flushed before taking its screen shot.)
Attachment #8881015 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #11)
> I'm a bit curious why this bug only appears when restyling, and not on the initial style.  Do you know why?

I wasn't asked, but:
Initially the font (and video) is already larger than without stylo. Each time I click on "Download", and X that dialog asking me for the download quality, all fonts get bigger. (and bigger...and bigger..)
(Assignee)

Comment 14

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #11)
> I'm a bit curious why this bug only appears when restyling, and not on the
> initial style.  Do you know why?

This is because when it happens on the initial style, we still don't have the root font size set on the Device (we have it set to the initial one), so it happens to work...
(Assignee)

Comment 15

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #12)
> Comment on attachment 8881015 [details]
> Bug 1375930: Test that rem units for the root font-size aren't resolved
> against the root font size.
> 
> https://reviewboard.mozilla.org/r/152384/#review157466
> 
> r=me but this test feels like it's veering into the realm of testing for a
> browser specific bug, so might be more appropriate in layout/reftests/bugs/
> or somewhere.

I always try to upstream everything if the test is reusable by other engines. There's precedence for that (see [1], for example). Is there a particular policy to not do that?

[1]: https://github.com/servo/servo/pull/14989#issuecomment-273042665
I don't think there's a particular policy, though taken to extremes, there are probably obscure crashtests we have that wouldn't provide much value in upstreaming.

That said, it's fine to upstream this one if you want (and it seems Florian agrees in general!). :-)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8880952 - Attachment is obsolete: true

Comment 18

2 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/60e8b4b68984
Test that rem units for the root font-size aren't resolved against the root font size. r=heycam

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/60e8b4b68984
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I wouldn't say it's fixed because it seems that only the tests are integrated now, but not a fix (?).
This seems to be fixed in Build 20170627100221 on Debian Testing x64 with webrender+webrendest+stylo enabled.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

2 years ago
Summary: stylo: site issue: large fonts and oversized video on zdf.de → stylo: site issue: large fonts on zdf.de
(Reporter)

Updated

2 years ago
See Also: → bug 1376788
You need to log in before you can comment on or make changes to this bug.