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)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Bug 1341775 - Part 1: stylo: Calculate font-size keywords based on base size;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341775 - Part 2: stylo: Update test expectations;
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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.
This should be super easy to fix and will unblock an unknown quantity of tests.
Assignee: nobody → manishearth
Priority: -- → P1
Prerequisite refactoring for this in https://github.com/servo/servo/pull/16016
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.
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.
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.
Agh, my solution doesn't take into account the fact that the keywordness still needs to be inherited. Fixing.
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)
Attachment #8848989 - Flags: review?(xidorn+moz) → review?(cam)
Attachment #8848990 - Flags: review?(xidorn+moz) → review?(cam)
Attachment #8849002 - Flags: review?(xidorn+moz) → review?(cam)
This seems to be more complicated than what I can review confidently...
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+
Attachment #8849002 - Flags: review?(xidorn+moz) → review?(cam)
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 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 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 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 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?
> 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 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)
(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. :-)
> 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.
> 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.
Attachment #8849002 - Attachment is obsolete: true
Attachment #8849002 - Flags: review?(cam)
Attachment #8849703 - Attachment is obsolete: true
(Rebased over tip, removed first and last patch)
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 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+
Attachment #8848989 - Attachment is obsolete: true
Attachment #8848990 - Attachment is obsolete: true
Servo side at https://github.com/servo/servo/pull/16122 . land this when that lands.
hg error in cmd: hg identify upstream -r tip:
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
NI glob re comment 60.
Flags: needinfo?(glob)
Status: NEW → ASSIGNED
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
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)
Depends on: 1351200
Blocks: 1352277
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: