Closed Bug 1375930 Opened 5 years ago Closed 5 years ago

stylo: site issue: large fonts on zdf.de

Categories

(Core :: CSS Parsing and Computation, defect)

56 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jan, Assigned: emilio)

References

(Blocks 1 open bug, )

Details

(Keywords: nightly-community)

Attachments

(3 files, 1 obsolete file)

Attached image 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.
Attached image 2017-06-23_zdf.de_2.png
Has STR: --- → yes
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Summary: stylo: site issue: zdf.de → stylo: site issue: large fonts and oversized video on zdf.de
This is going to be a rem units issue, potentially similar to bug 1374062.
Gah, got it.
Assignee: nobody → emilio+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Thanks for all the reports btw, they're awesome!
I'm actually not sure how we've had this so wrong for so long... I bet this is causing some tests to fail.
(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 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 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..)
(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...
(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!). :-)
Attachment #8880952 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/60e8b4b68984
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
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
Summary: stylo: site issue: large fonts and oversized video on zdf.de → stylo: site issue: large fonts on zdf.de
See Also: → 1376788
You need to log in before you can comment on or make changes to this bug.