stylo: MSN.com text and images are too big

VERIFIED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
5 months ago
5 months ago

People

(Reporter: cpeterson, Assigned: emilio)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 months ago
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.
It seems they use rem for sizing. I think we have had some bugs for that...
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.
Anyway, seems to be font-size related, cc manish.
Emilio recently redid how rem works
(Assignee)

Comment 5

5 months ago
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...
Assignee: nobody → emilio+bugs
(Assignee)

Comment 6

5 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

5 months ago
mozreview-review
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.
Attachment #8884368 - Flags: review?(cam) → review+

Comment 12

5 months ago
mozreview-review
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.
Attachment #8884369 - Flags: review?(cam) → review+

Comment 13

5 months ago
mozreview-review
Comment on attachment 8884370 [details]
Bug 1379041: Reftest.

https://reviewboard.mozilla.org/r/155294/#review160460
Attachment #8884370 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8884368 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8884369 - Attachment is obsolete: true

Comment 15

5 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f0d0de89d505
Reftest. r=heycam
(Assignee)

Comment 16

5 months ago
Servo bits landed in https://github.com/servo/servo/issues/17639.
https://hg.mozilla.org/mozilla-central/rev/f0d0de89d505
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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
Status: RESOLVED → VERIFIED
Has STR: --- → yes
Version: unspecified → Trunk
Duplicate of this bug: 1378208
(Assignee)

Updated

5 months ago
See Also: → bug 1379505
You need to log in before you can comment on or make changes to this bug.