Closed
Bug 1380980
Opened 7 years ago
Closed 7 years ago
stylo: font_size::SpecifiedValue::as_font_ratio looks super-fishy.
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
People
(Reporter: emilio, Assigned: manishearth)
References
Details
Attachments
(4 files)
That method reads like: pub fn as_font_ratio(&self) -> Option<f32> { if let SpecifiedValue::Length(ref lop) = *self { if let LengthOrPercentage::Percentage(pc) = *lop { return Some(pc.0) } else if let LengthOrPercentage::Length(ref nocalc) = *lop { if let NoCalcLength::FontRelative(FontRelativeLength::Em(em)) = *nocalc { return Some(em) } } } else if let SpecifiedValue::Larger = *self { return Some(LARGER_FONT_SIZE_RATIO) } else if let SpecifiedValue::Smaller = *self { return Some(1. / LARGER_FONT_SIZE_RATIO) } None } It only checks LengthOrPercentage::Length and LengthOrPercentage::Percentage, but as far as I can tell, there's nothing preventing us from arriving there with a LengthOrPercentage::Calc, in which case the font ratio won't be detected or honored. I can see why handling calc is annoying (is it even possible to do so with our current setup?).
Assignee | ||
Comment 2•7 years ago
|
||
No, this looks like an oversight. It's not possible to do with the current setup, however, you will have to track an additive bit and a factor in the current setup.
Flags: needinfo?(manishearth)
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8906249 [details] Bug 1380980 - stylo: Add (unused) absolute offset to FontComputationData; https://reviewboard.mozilla.org/r/178002/#review183136 r=me, assuming it's useful for following commits. ::: servo/components/style/properties/properties.mako.rs:2550 (Diff revision 1) > /// The writing mode flags. > /// > /// TODO(emilio): Make private. > pub writing_mode: WritingMode, > /// The keyword behind the current font-size property, if any. > - pub font_size_keyword: Option<(longhands::font_size::KeywordSize, f32)>, > + pub font_size_keyword: Option<(longhands::font_size::KeywordSize, f32, NonNegativeAu)>, nit: Can we make this a struct, please?
Attachment #8906249 -
Flags: review?(emilio) → review+
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8906250 [details] Bug 1380980 - stylo: Add support for calcs in base size calculation; https://reviewboard.mozilla.org/r/178004/#review183138 ::: servo/components/style/properties/longhand/font.mako.rs:625 (Diff revision 1) > - /// A keyword value, along with a ratio. > + /// A keyword value, along with a ratio and absolute offset. > /// The ratio in any specified keyword value > - /// will be 1, but we cascade keywordness even > + /// will be 1 (with offset 0), but we cascade keywordness even > /// after font-relative (percent and em) values > /// have been applied, which is where the keyword > /// comes in. See bug 1355707 Please describe when the offset thingie may be non-zero. ::: servo/components/style/properties/longhand/font.mako.rs:829 (Diff revision 1) > } > - // FIXME(emilio): This looks super fishy! > - LengthOrPercentage::Calc(..) => None, > + LengthOrPercentage::Calc(ref calc) => { > + if calc.em.is_none() && calc.percentage.is_none() { > + return None; > - } > + } > + let ratio = calc.em.unwrap_or(0.) + calc.percentage.map(|pc| pc.0).unwrap_or(0.); nit: map_or instead of map(..).unwrap_or? ::: servo/components/style/properties/longhand/font.mako.rs:831 (Diff revision 1) > - LengthOrPercentage::Calc(..) => None, > + if calc.em.is_none() && calc.percentage.is_none() { > + return None; > - } > + } > + let ratio = calc.em.unwrap_or(0.) + calc.percentage.map(|pc| pc.0).unwrap_or(0.); > + // Compute it, but shave off the font-relative part > + let abs = calc.to_computed_value_zoomed(context, FontBaseSize::Custom(Au(0).into())) I'm not convinced this is correct for font-dependent units. In particular, if we have something like `font-size: calc(10ex + 1em)`, and a style inheriting from it would have another `font-family` wouldn't the `10ex` offset computed with the _old_ font family end up somewhere in the calculation of the new size? Is that expected? If so, why is that correct? The commit message could have some love too :)
Attachment #8906250 -
Flags: review?(emilio)
Assignee | ||
Comment 9•7 years ago
|
||
> wouldn't the `10ex` offset computed with the _old_ font family end up somewhere in the calculation of the new size Yes. > Is that expected? No. > If so, why is that correct? Because it's a super edge case and there's no right answer here. We can match what Gecko does (which is inefficient), or do this. My eventual plan is to change Gecko's code to follow this model too.
Reporter | ||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8906251 [details] Bug 1380980 - stylo: Add test; https://reviewboard.mozilla.org/r/178006/#review183140 r=me. It'd be nice to have tests for this with other kinds of units / etc.
Attachment #8906251 -
Flags: review?(emilio) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8906250 [details] Bug 1380980 - stylo: Add support for calcs in base size calculation; https://reviewboard.mozilla.org/r/178004/#review183568 r=me
Attachment #8906250 -
Flags: review?(emilio) → review+
Reporter | ||
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8906785 [details] Bug 1380980 - stylo: Replace FontComputationData with typedef; https://reviewboard.mozilla.org/r/178516/#review183570 ::: servo/components/style/properties/properties.mako.rs:113 (Diff revision 1) > - /// and is 1 when there was just a keyword and no relative values. > +/// and is 1 when there was just a keyword and no relative values. > - /// > +/// > - /// When this is Some, we compute font sizes by computing the keyword against > +/// When this is Some, we compute font sizes by computing the keyword against > - /// the generic font, and then multiplying it by the ratio (as well as adding any > +/// the generic font, and then multiplying it by the ratio (as well as adding any > - /// absolute offset from calcs) > +/// absolute offset from calcs) > - pub font_size_keyword: Option<(longhands::font_size::KeywordSize, f32, NonNegativeAu)> > +pub type FontComputationData = Option<(longhands::font_size::KeywordSize, f32, NonNegativeAu)>; Hmm, I meant more like: ``` struct FontComputationData { keyword: KeywordSize, factor: f32, offset: NonNegativeAu, } ``` Then using `Option<FontComputationData>`. But I guess this works too?
Attachment #8906785 -
Flags: review?(emilio) → review+
Comment 17•7 years ago
|
||
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/bf6b321ede7b stylo: Add (unused) absolute offset to FontComputationData; r=emilio https://hg.mozilla.org/integration/autoland/rev/2dc8bf7c38ae stylo: Add test; r=emilio
Comment 18•7 years ago
|
||
This added a word to ComputedValues (264 -> 272). Did we really need to do that, or is there some clever way we could pack this better? Adding a word here costs us half a megabyte on the html spec.
Flags: needinfo?(manishearth)
Comment 19•7 years ago
|
||
Pushed by philringnalda@gmail.com: https://hg.mozilla.org/integration/autoland/rev/327f2a97631d followup, mark test as failing on QuantumRender
Updated•7 years ago
|
Keywords: leave-open
Comment 20•7 years ago
|
||
Backed out my annotation and the whole test in https://hg.mozilla.org/integration/autoland/rev/24c584c7450a since apparently it fails either everywhere, or a whole lot of wheres, and we just hardly ever run reftests because we're too cheap to run them more than once every dozen or two pushes.
Assignee | ||
Comment 21•7 years ago
|
||
I suspect it fails because the font prefs may be different across platforms. I'll investigate.
Flags: needinfo?(manishearth)
Assignee | ||
Comment 22•7 years ago
|
||
> This added a word to ComputedValues (264 -> 272). Did we really need to do that, or is there some clever way we could pack this better? Adding a word here costs us half a megabyte on the html spec.
Bear in mind this is handling somewhat of an edge case so we could even *not* include the fix. I'm not sure what others feel about this. I'm ambivalent.
Some solutions are:
- Making it a u16 and somehow getting rustc to pack it with the KeywordSize. This reduces the total range it can go, but this is an edge case.
- Moving the whole thing to the nsStyleFont. This is where it *should* be, and where I intend to move it once the Gecko style system is removed. However this will bloat nsStyleFont (but that gets shared more, presumably). If it's on nsStyleFont there are other packing opportunities, e.g. we can pack it with the script size multiplier or mathvariant fields.
- Switching over to Gecko's solution; i.e. walking up the rule tree and recalculating styles as if the base size were different. I do not recommend this; it's super inefficient and requires us to overcomplicate the code.
- We only have two unused bits on the Au (max value shaves off one bit, and it's non negative), so not enough to store Option<KeywordSize>. We can reduce the range of the offset to (1<<27 - 1) or something and then bitpack it.
Thoughts?
Comment 23•7 years ago
|
||
Per discussion in the meeting, moving to nsStyleFont is preferable. Even better if we can get it to use one word there instead of two. File a bug? I think this would be nice to do in the 57 timeframe, since it would be a 6% reduction in the size of ComputedValues, which in turn is a large part of our memory overhead.
Assignee | ||
Comment 24•7 years ago
|
||
I think I can get it done by 57. Probably is straightforward, just potentially tedious. Filed bug 1399228.
Assignee | ||
Comment 25•7 years ago
|
||
The test failures are because for some reason the base font-size for monospace is 12px instead of 13. We need to find something that doesn't vary across platforms in the default configuration.
Updated•7 years ago
|
status-firefox55:
--- → wontfix
status-firefox56:
--- → wontfix
status-firefox57:
--- → affected
status-firefox-esr52:
--- → wontfix
Assignee | ||
Comment 26•7 years ago
|
||
This actually already landed; the test didn't. I'm unsure how to test this reliably since the prefs differ. Perhaps we can only run the test on one platform?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Comment 27•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•