stylo: need to do the right thing with "font-family: monospace" in terms of font size

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: bz, Assigned: manishearth)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 4 obsolete attachments)

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
(Assignee)

Comment 2

3 months ago
Prerequisite refactoring for this in https://github.com/servo/servo/pull/16016
(Assignee)

Comment 3

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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)
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 18

3 months 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)
Attachment #8849002 - Flags: review?(xidorn+moz) → review?(cam)

Comment 24

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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)
(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.
(Assignee)

Comment 43

3 months 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

3 months ago
Attachment #8849002 - Attachment is obsolete: true
Attachment #8849002 - Flags: review?(cam)
(Assignee)

Updated

3 months ago
Attachment #8849703 - Attachment is obsolete: true
(Assignee)

Comment 52

3 months ago
(Rebased over tip, removed first and last patch)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 55

3 months 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

3 months 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

3 months ago
Attachment #8848989 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8848990 - Attachment is obsolete: true
(Assignee)

Comment 59

3 months ago
Servo side at https://github.com/servo/servo/pull/16122 . land this when that lands.

Comment 60

3 months ago
hg error in cmd: hg identify upstream -r tip:

Comment 61

3 months 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
NI glob re comment 60.
Flags: needinfo?(glob)
(Assignee)

Updated

3 months ago
Status: NEW → ASSIGNED

Comment 63

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae03919554fb
https://hg.mozilla.org/mozilla-central/rev/b166eb78c7ec
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months 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
(Assignee)

Updated

3 months ago
Blocks: 1352277
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1335574

Updated

a month ago
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.