1375930 - stylo: site issue: large fonts on zdf.de

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Darkspirit]

Attachment: 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.

Comment 1

[Darkspirit]

Attachment: 2017-06-23_zdf.de_2.png

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This is going to be a rem units issue, potentially similar to bug 1374062.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Gah, got it.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Thanks for all the reports btw, they're awesome!

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1375930: Fix rem computation on the root element.

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...

Review commit: https://reviewboard.mozilla.org/r/152322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152322/

Comment 7

[Darkspirit]

(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 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1375930: Test that rem units for the root font-size aren't resolved against the root font size.

Of course there wasn't any test for this.

Review commit: https://reviewboard.mozilla.org/r/152384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152384/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8880952 [details]
Bug 1375930: Fix rem computation on the root element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152322/diff/1-2/

Comment 10

[Cameron McCormack (:heycam)]

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.

Comment 11

[Cameron McCormack (:heycam)]

I'm a bit curious why this bug only appears when restyling, and not on the initial style.  Do you know why?

Comment 12

[Cameron McCormack (:heycam)]

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.)

Comment 13

[Darkspirit]

(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..)

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(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...

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(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

Comment 16

[Cameron McCormack (:heycam)]

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 17

[Emilio Cobos Álvarez (:emilio)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152384/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60e8b4b68984

Comment 20

[Darkspirit]

I wouldn't say it's fixed because it seems that only the tests are integrated now, but not a fix (?).

Comment 21

[Emilio Cobos Álvarez (:emilio)]

The actual fix landed at https://github.com/servo/servo/pull/17516.

Comment 22

[Darkspirit]

This seems to be fixed in Build 20170627100221 on Debian Testing x64 with webrender+webrendest+stylo enabled.