Closed Bug 1374062 Opened 8 years ago Closed 8 years ago

stylo: We're too liberal setting the root font-size.

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1374062: Test.
59 bytes, text/x-review-board-request
heycam
: review+
Details
STR: Go to https://www.wikipedia.org/, and select in the language dropdown, or click on the "Read Wikipedia in your language" dropdown. The font-size starts jumping around. The button/select/optgroup have a rule with font: inherit, which i suspect to be the culprit here, though I haven't investigated further.
Flags: needinfo?(manishearth)
Actually, this affects to other incremental updates to font-size. The links around the globe have "font-size: larger". Recascading the whole document (resizing the window as of right now) makes them bigger.
Summary: stylo: Explicitly inherited font seems broken (affects wikipedia). → stylo: Incremental font-size updates seem broken (affects wikipedia).
This is on a build of Jun 8, btw, but I haven't seen anything relevant land in the meantime.
After investigating a bit more I'm pretty sure this isn't because of font-size, but because of rem. Something in that <select> element is making us update the root font-size to be 16px instead of whatever wikipedia uses, and when recalculating style of the elements we use the wrong root font-size later.
Potential fix is checking the owner_doc() of the root element, against the one in the pres context but I still want to get a test-case... I'm not sure I'll be able to investigate today though.
Oh. Gah. I know what the bug is, but I thought we'd fixed that. Perhaps we never fixed it for restyles? Basically, the root size is cached on the device, but select and all have their own root elements sharing the device. But we propagate this information as a cascade flag and don't set root fs when were in a fake root tree. This flag/machinery may be missing from restyles.
It is not, that flag is right. The problem is how we assume that the root element == anything which doesn't inherit from nothing. Which is quite wrong. I have a fix and I'm working on a test-case.
Flags: needinfo?(manishearth)
Here's a test-case, sorry for the lag: <!doctype html> <style> :root { font-size: 5px; } [restyled] { color: green; font-size: 2rem; } </style> <div> Should be green, and have a 10px font-size. </div> <script> getComputedStyle(document.createElement('div')).color; document.querySelector('div').setAttribute("restyled", ""); </script>
Assignee: nobody → emilio+bugs
Summary: stylo: Incremental font-size updates seem broken (affects wikipedia). → stylo: We're too liberal setting the root font-size.
I think this may still not be correct if you call getComputedStyle on another window with the root element of another document, I'll try to build a test case and fix.
(I'm not sure what we even try to do when doing getComputedStyle across documents...)
Comment on attachment 8879896 [details] Bug 1374062: Assert we never style a root element from another document. https://reviewboard.mozilla.org/r/151302/#review156178 ::: commit-message-e5860:3 (Diff revision 1) > +Before this commit, we assumed that if the element had no parent element, it was > +the root of the document, which is plain false, since we can arrive there from, > +let's say, getComputedStyle on a detached node. To be fair, we *did* correctly handle the case of document-level NAC, which also has no parent element. But yes, roots of disconnected subtrees are a big one to have missed. :-) ::: servo/components/style/properties/properties.mako.rs:2483 (Diff revision 1) > /// Not set for native anonymous content since some NAC > /// form their own root, but share the device. > /// > /// ::backdrop and all NAC will resolve rem units against > /// the toplevel root element now. May as well replace this with some wording about how the flag influences some style adjustments and also how we determine the root font-size for rem and units.
Attachment #8879896 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #11) > To be fair, we *did* correctly handle the case of document-level NAC, which > also has no parent element. But yes, roots of disconnected subtrees are a > big one to have missed. :-) (Actually, that's not true. Document-level NAC does have the root element as its parent.)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10) > (I'm not sure what we even try to do when doing getComputedStyle across > documents...) We restyle using the owner doc's pres context, so this is handled correctly, for now. I added a second patch with an assertion.
Mozreview got really confused :(.
Gah, and it still got the patches in the wrong order :(.
Comment on attachment 8879896 [details] Bug 1374062: Assert we never style a root element from another document. https://reviewboard.mozilla.org/r/151302/#review156290 ::: servo/components/style/gecko/media_queries.rs:109 (Diff revision 3) > pub fn set_root_font_size(&self, size: Au) { > self.root_font_size.store(size.0 as isize, Ordering::Relaxed) > } > > + /// Gets the pres context associated with this document. > + pub fn pres_context(&self) -> &nsPresContext { if this function is safe, `Device::new` must be unsafe.
Comment on attachment 8879903 [details] Bug 1374062: Test. https://reviewboard.mozilla.org/r/151308/#review156590 ::: servo/components/style/properties/properties.mako.rs:2483 (Diff revision 2) > /// Not set for native anonymous content since some NAC > /// form their own root, but share the device. > /// > /// ::backdrop and all NAC will resolve rem units against > /// the toplevel root element now. As mentioned in comment 11, consider updating this comment.
Attachment #8879903 - Flags: review?(cam) → review+
Attachment #8879896 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0a7160e976d8
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: