Closed
Bug 1379041
Opened 7 years ago
Closed 7 years ago
stylo: MSN.com text and images are too big
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: cpeterson, Assigned: emilio)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 2 obsolete files)
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•7 years ago
|
||
It seems they use rem for sizing. I think we have had some bugs for that...
Comment 2•7 years ago
|
||
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•7 years ago
|
||
Anyway, seems to be font-size related, cc manish.
Comment 4•7 years ago
|
||
Emilio recently redid how rem works
Assignee | ||
Comment 5•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Attachment #8884368 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8884369 -
Attachment is obsolete: true
Comment 15•7 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f0d0de89d505
Reftest. r=heycam
Assignee | ||
Comment 16•7 years ago
|
||
Servo bits landed in https://github.com/servo/servo/issues/17639.
Comment 17•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 18•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•