Closed
Bug 1341775
Opened 7 years ago
Closed 7 years ago
stylo: need to do the right thing with "font-family: monospace" in terms of font size
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: manishearth)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
The default font-size is different for monospace vs non-monospace fonts (13px vs 16px). Looks like stylo always makes it 16px. This breaks various reftests, and user expectations.
Comment 1•7 years ago
|
||
This should be super easy to fix and will unblock an unknown quantity of tests.
Assignee: nobody → manishearth
Priority: -- → P1
Assignee | ||
Comment 2•7 years ago
|
||
Prerequisite refactoring for this in https://github.com/servo/servo/pull/16016
Assignee | ||
Comment 3•7 years ago
|
||
This introduces a more complicated dependence of properties than our "early properties" framework was previously equipped to handle. Previously, we had "early" properties and "late" properties, where the early ones are ones like writing-mode, font-size, font-family, etc which other properties need during computation. Now, font-family is a dependency of font-size, which itself is a dependency of all the late properties. To avoid an extra iteration I'm going to pull out the font-size and family properties during the early computation and compute them separately, in order, before the late computation. I'm pulling out the family property as well since the internal system font property (which I may work on next) is going to have to be early-computed, before family.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
Part 1 is the same as https://github.com/servo/servo/pull/16016, doesn't need to be reviewed here -- review it there first and I'll rebase later. In part 3 we call nsPresContext::GetDefaultFont, which might not be thread safe -- we need to figure that out.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
This doesn't handle the fact that the initial /computed value/ is "medium". We could make font-size compute to "Option<Au>" with None as the default and then do the computation of the None case in gecko.mako.rs. But then em units get tricky, and this sounds like it would be expensive, too.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
Agh, my solution doesn't take into account the fact that the keywordness still needs to be inherited. Fixing.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8848988 [details] Bug 1341775 - Part 1: stylo: Calculate font-size keywords based on base size; https://reviewboard.mozilla.org/r/121842/#review124254 Seems to be the landed part in Servo. Cancel review here.
Attachment #8848988 -
Flags: review?(xidorn+moz)
Updated•7 years ago
|
Attachment #8848989 -
Flags: review?(xidorn+moz) → review?(cam)
Updated•7 years ago
|
Attachment #8848990 -
Flags: review?(xidorn+moz) → review?(cam)
Attachment #8849002 -
Flags: review?(xidorn+moz) → review?(cam)
Comment 17•7 years ago
|
||
This seems to be more complicated than what I can review confidently...
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8848989 [details] Bug 1341775 - Part 3: stylo: Calculate font-size keywords based on base size; https://reviewboard.mozilla.org/r/121844/#review124286
Attachment #8848989 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8849002 -
Flags: review?(xidorn+moz) → review?(cam)
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8848990 [details] Bug 1341775 - Part 4: stylo: Special-case initial-computation of font-size; https://reviewboard.mozilla.org/r/121846/#review124290 ::: layout/style/ServoBindings.cpp:1475 (Diff revision 3) > } > > +int32_t > +Gecko_nsStyleFont_GetBaseSize(const nsStyleFont* aFont, RawGeckoPresContextBorrowed aPresContext) > +{ > + return (int32_t) aPresContext->GetDefaultFont(aFont->mGenericID, Nit: this cast isn't needed. (nscoord is an int32_t anyway, unless NS_COORD_IS_FLOAT is set, though that's likely to be a broken configuration in Gecko anyway.) Actually, given we have other FFI functions that take and return nscoord values, let's just make this function return nscoord. ::: servo/components/style/properties/gecko.mako.rs:1209 (Diff revision 3) > + (FontFamilyType::eFamily_moz_fixed, > + structs::kGenericFont_moz_fixed) > + } else { > + panic!("Unknown generic font family") > + }; > + if v.0.len() == 1 { Why do we check this? ::: servo/components/style/properties/longhand/font.mako.rs:510 (Diff revision 3) > + type ComputedValue = Au; > + #[inline] > + fn to_computed_value(&self, cx: &Context) -> computed_value::T { > + use gecko_bindings::bindings::Gecko_nsStyleFont_GetBaseSize; > + use values::specified::length::au_to_int_px; > + // Data from nsRuleNode.cpp in Gecko Is it worth trying to share this data, by making sStrictFontSizeTable, sQuirksFontSizeTable and sFontSizeFactors static consts, which we could bindgen and use? I think it would be nice to avoid duplicating, if we can do it easily.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8849002 [details] Bug 1341775 - Part 4: stylo: Special-case initial-computation of font-size; https://reviewboard.mozilla.org/r/121856/#review125260 I don't really like the special cascading/computation behaviour for font-size here. If we stored the keyword on the Gecko nsStyleFont struct, how much would that simplify this patch?
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8849702 [details] Bug 1341775 - Part 2: stylo: Update test expectations; https://reviewboard.mozilla.org/r/122476/#review125262 rs=me
Attachment #8849702 -
Flags: review?(cam) → review+
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8849703 [details] Bug 1341775 - Part 6: stylo: Ensure float is computed after position; https://reviewboard.mozilla.org/r/122478/#review125266 Seems like this belongs in a different bug? r=me on it, though. ::: servo/components/style/properties/properties.mako.rs:2073 (Diff revision 3) > &mut context, > &mut cacheable, > &mut cascade_info, > error_reporter); > } > + // float must be computed after position, but before display This reminds me, should we be setting mOriginalFloat like we do mOriginalDisplay?
Attachment #8849703 -
Flags: review?(cam) → review+
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8849002 [details] Bug 1341775 - Part 4: stylo: Special-case initial-computation of font-size; https://reviewboard.mozilla.org/r/121856/#review125268 ::: servo/components/style/properties/longhand/font.mako.rs:573 (Diff revision 6) > + pub static MEDIUM_DECLARATION: PropertyDeclaration = > + PropertyDeclaration::FontSize(DeclaredValue::Value( > + SpecifiedValue::Keyword(Medium) > + )); > + What is this used for?
Assignee | ||
Comment 35•7 years ago
|
||
> I don't really like the special cascading/computation behaviour for font-size here. If we stored the keyword on the Gecko nsStyleFont struct, how much would that simplify this patch? It wouldn't. It would have to do the same thing. A better fix is to make to_computed_value operate on a mutable context -- then we can store it wherever we want and the helpers wouldn't be special cased -- but I would prefer to keep things like that immutable. > Is it worth trying to share this data meh. it's a small table. > Why do we check this? because gecko only sets the generic id when the font list is a single generic font. see nsRuleNode::ComputeFontData > This reminds me, should we be setting mOriginalFloat like we do mOriginalDisplay? I think. Followup bug? > What is this used for? Was used in a previous iteration, now unused, my bad.
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8848988 [details] Bug 1341775 - Part 1: stylo: Calculate font-size keywords based on base size; https://reviewboard.mozilla.org/r/121842/#review125272
Attachment #8848988 -
Flags: review?(cam)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 41•7 years ago
|
||
(In reply to Manish Goregaokar [:manishearth] from comment #35) > It wouldn't. It would have to do the same thing. A better fix is to make > to_computed_value operate on a mutable context -- then we can store it > wherever we want and the helpers wouldn't be special cased -- but I would > prefer to keep things like that immutable. But couldn't we move the handling of the font_size_keyword value (i.e. its inheritance, and its influence on computing font-size itself) into the set_font_size/copy_font_size_from functions, and not need this custom cascading code? > > Is it worth trying to share this data > > meh. it's a small table. I am thinking more from the code maintainability point of view, rather than effect on binary size. (Though I guess once the Gecko style system goes away this doesn't matter.) > > Why do we check this? > > because gecko only sets the generic id when the font list is a single > generic font. see nsRuleNode::ComputeFontData Ah, got it. (I don't actually know the reason behind this, though.) > I think. Followup bug? Yes, please. :-)
Reporter | ||
Comment 42•7 years ago
|
||
> This reminds me, should we be setting mOriginalFloat like we do mOriginalDisplay? No. See bug 1342738 comment 3. We may want a comment explaining this, though.
Assignee | ||
Comment 43•7 years ago
|
||
> But couldn't we move the handling of the font_size_keyword value (i.e. its inheritance, and its influence on computing font-size itself) into the set_font_size/copy_font_size_from functions Yes, we could; kind of. We still need to tweak how initial values are handled and a bunch of other things. This was an alternative I was considering, but finally decided against it -- it's roughly the same amount of special casing. We're doing a slightly-custom cascade for system fonts (bug 1349417) anyway (I discussed this and folks seemed to prefer caching it on ComputedValues instead of GeckoFont), and this lets us be more consistent about this kind of code.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8849002 -
Attachment is obsolete: true
Attachment #8849002 -
Flags: review?(cam)
Assignee | ||
Updated•7 years ago
|
Attachment #8849703 -
Attachment is obsolete: true
Assignee | ||
Comment 52•7 years ago
|
||
(Rebased over tip, removed first and last patch)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 55•7 years ago
|
||
mozreview-review |
Comment on attachment 8848988 [details] Bug 1341775 - Part 1: stylo: Calculate font-size keywords based on base size; https://reviewboard.mozilla.org/r/121842/#review125692
Attachment #8848988 -
Flags: review?(cam) → review+
Comment 56•7 years ago
|
||
mozreview-review |
Comment on attachment 8848990 [details] Bug 1341775 - Part 4: stylo: Special-case initial-computation of font-size; https://reviewboard.mozilla.org/r/121846/#review125694
Attachment #8848990 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8848989 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8848990 -
Attachment is obsolete: true
Assignee | ||
Comment 59•7 years ago
|
||
Servo side at https://github.com/servo/servo/pull/16122 . land this when that lands.
Comment 60•7 years ago
|
||
hg error in cmd: hg identify upstream -r tip:
Comment 61•7 years ago
|
||
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ae03919554fb Part 1: stylo: Calculate font-size keywords based on base size; r=heycam https://hg.mozilla.org/integration/autoland/rev/b166eb78c7ec Part 2: stylo: Update test expectations; r=heycam
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 63•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae03919554fb https://hg.mozilla.org/mozilla-central/rev/b166eb78c7ec
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 64•7 years ago
|
||
thanks bobby. > hg error in cmd: hg identify upstream -r tip: this was caused by transient networking issues with scl3. several services were experiencing issues, including BMO. filed bug 1350801 to fix the error message.
Flags: needinfo?(glob)
Updated•7 years ago
|
status-firefox54:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•