1379041 - stylo: MSN.com text and images are too big

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Load www.msn.com/en-us/news/good-news/
2. Scroll down the page

RESULT:
Everything on the page is too big: text, images, space between headlines.

Similarly:

1. Load http://www.msn.com/en-us/news/good-news/we-love-what-this-group-did-with-flowers-that-were-headed-for-the-trash/ar-BBDO2b9
2. Scroll down the page

RESULT:
The social media sharing buttons in the left column are too big and overlap the article text. Some of the article images are stretched too tall.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

It seems they use rem for sizing. I think we have had some bugs for that...

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Also, when I save the page locally, and strip all script, then this issue doesn't reproduce anymore.

So there must be some dynamic change involves.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Anyway, seems to be font-size related, cc manish.

Comment 4

[Manish Goregaokar [:manishearth]]

Emilio recently redid how rem works

Comment 5

[Emilio Cobos Álvarez (:emilio)]

What? I only fixed a bunch of bugs that https://github.com/servo/servo/pull/17041 left behind, the story is basically the same.

I can take a look though, I guess...

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So this is due to us updating the rem units from getDefaultComputedStyle. The fix is easy enough, I just need to construct a proper test-case...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Move root font size handling outside of the cascade.

And merge it with the rest of the code handling root font-size changes.

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Avoid overriding the root font size from a getDefaultComputedStyle call.

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1379041: Reftest.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8884370 [details]
Bug 1379041: Reftest.

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

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8884368 [details]
style: Move root font size handling outside of the cascade.

https://reviewboard.mozilla.org/r/155290/#review160456

::: servo/components/style/matching.rs:557
(Diff revision 1)
>                  // The new root font-size has already been updated on the Device
>                  // in properties::apply_declarations.

This comment is no longer accurate and can be removed.

::: servo/components/style/matching.rs:562
(Diff revision 1)
>                  // If the root font-size changed since last time, and something
>                  // in the document did use rem units, ensure we recascade the
>                  // entire tree.

Maybe move this comment to just above the `if device.used_root_font_size()`, now that this block not only does this but also updates the root font-size on the Device.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8884369 [details]
style: Avoid overriding the root font size from a getDefaultComputedStyle call.

https://reviewboard.mozilla.org/r/155292/#review160458

I wonder if there is any other state that we update during a getDefaultComputedStyle that we shouldn't be...

::: servo/components/style/matching.rs:557
(Diff revision 1)
> +            // TODO(emilio): This should arguably be outside of the patch for
> +            // getComputedStyle/getDefaultComputedStyle, but it's unclear how to
> +            // do it without duplicating a bunch of code.

Should it be "path" rather than "patch"?  Otherwise I'm not sure I understand the comment.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8884370 [details]
Bug 1379041: Reftest.

https://reviewboard.mozilla.org/r/155294/#review160460

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8884370 [details]
Bug 1379041: Reftest.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0d0de89d505
Reftest. r=heycam

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Servo bits landed in https://github.com/servo/servo/issues/17639.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f0d0de89d505

Comment 18

[Darkspirit]

Nightly 56 x64 20170709100223 @ Debian Testing (Linux 4.9.0-3-amd64, Radeon RX480)
Verified fixed:
* The social sharing buttons are now small as they should be on http://www.msn.com/en-us/news/good-news/we-love-what-this-group-did-with-flowers-that-were-headed-for-the-trash/ar-BBDO2b9
* http://www.msn.com/en-us/news/good-news/ is now like in Google Chrome