stylo: Properly support -moz-script-size-multiplier, -moz-script-level, and -moz-script-min-size

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

(Blocks: 1 bug)

unspecified
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

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

2 months ago
These three properties affect the font size (see ComputeScriptLevelSize and its callers in nsRuleNode.cpp).

Should make most of the reftests from [1] pass.


 [1]: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/dUbOmVtcSQiAVOZxXjjbMw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
Over to Fernando. Looks like this this affects quite a number of tests.
Assignee: nobody → ferjmoreno
Priority: -- → P1
(Assignee)

Comment 2

2 months ago
Already have a fix for this, didn't realize this was assigned.

I've also fixed how inherit works, previously

    <span style="font-size: inherit; font-family: monospace;">b</span><span style="font-family: monospace;">c</span>

would not give the same font size for both values.
Assignee: ferjmoreno → manishearth
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 months ago
mozreview-review
Comment on attachment 8858199 [details]
Bug 1355427 - Part 1: stylo: Support computing font-size against an arbitrary base size;

https://reviewboard.mozilla.org/r/130144/#review132794

::: servo/components/style/properties/longhand/font.mako.rs:658
(Diff revision 1)
>              use values::specified::length::FontRelativeLength;
>              match *self {
>                  SpecifiedValue::Length(LengthOrPercentage::Length(
>                          NoCalcLength::FontRelative(value))) => {
> -                    value.to_computed_value(context, /* use inherited */ true)
> +                    value.to_computed_value(context,
> +                                            Some(context.inherited_style().get_font().clone_font_size()))

Can we use an enum like:

```
enum FontSizeBaseSize {
    CurrentStyle,
    InheritedStyle,
    Custom(Au),
}
```
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Manish, can you provide some detailed commit messages in these patches?  I wasn't part of the conversation with dbaron explaining how these work, and I'm going to have to re-read and work out what nsRuleNode is doing wrt these.  But for future code readers (and me when I come to review it), an explanation of why we need this functionality and what we're doing at a high level would be useful.  Thanks.
Flags: needinfo?(manishearth)
(Assignee)

Comment 9

2 months ago
Added both a commit message and a comment (identical)
Flags: needinfo?(manishearth)
Comment hidden (mozreview-request)

Comment 11

2 months ago
mozreview-review
Comment on attachment 8858199 [details]
Bug 1355427 - Part 1: stylo: Support computing font-size against an arbitrary base size;

https://reviewboard.mozilla.org/r/130144/#review134658

::: servo/components/style/properties/longhand/font.mako.rs:657
(Diff revision 2)
>                  SpecifiedValue::Length(LengthOrPercentage::Percentage(Percentage(value))) => {
> -                    context.inherited_style().get_font().clone_font_size().scale_by(value)
> +                    base_size.resolve(context).scale_by(value)
>                  }
>                  SpecifiedValue::Length(LengthOrPercentage::Calc(ref calc)) => {
>                      let calc = calc.to_computed_value(context);
> -                    calc.length() + context.inherited_style().get_font().clone_font_size()
> +                    calc.length() +base_size.resolve(context)

Nit: space after "+".

::: servo/components/style/values/computed/length.rs:28
(Diff revision 2)
>      fn to_computed_value(&self, context: &Context) -> Au {
>          match *self {
>              specified::NoCalcLength::Absolute(length) =>
>                  length.to_computed_value(context),
>              specified::NoCalcLength::FontRelative(length) =>
> -                length.to_computed_value(context, /* use inherited */ false),
> +                length.to_computed_value(context, /* base_size */ FontBaseSize::CurrentStyle),

Nit: I guess given the name FontBaseSize we probably don't need that comment any more.  (And below.)

::: servo/components/style/values/specified/length.rs:101
(Diff revision 2)
> -    /// Computes the font-relative length. We use the use_inherited flag to
> -    /// special-case the computation of font-size.
> +    /// Computes the font-relative length. We use the inherited_size
> +    /// flag to pass a different size for computing font-size and unconstrained font-size

Should that be s/inherited_size/base_size/?
Attachment #8858199 - Flags: review?(cam) → review+

Comment 12

2 months ago
mozreview-review
Comment on attachment 8858200 [details]
Bug 1355427 - Part 2: stylo: Support scriptlevel computation and scriptminsize;

https://reviewboard.mozilla.org/r/130146/#review134656

::: commit-message-38c98:3
(Diff revision 3)
> +Bug 1355427 - Part 2: stylo: Support scriptlevel computation and scriptminsize; r?heycam
> +
> +scriptlevel is a property that affects how font-size is inherited. If scriptlevel is

Thanks for the awesome explanation!  If it is identical to the comment, then I think it's fine to drop it from the commit message and just leave it in the comment.

::: servo/components/style/properties/gecko.mako.rs:1327
(Diff revision 3)
>      // FIXME(bholley): Gecko has two different sizes, one of which (mSize) is the
>      // actual computed size, and the other of which (mFont.size) is the 'display
>      // size' which takes font zooming into account. We don't handle font zooming yet.

Please update this comment to briefly call out the third font size we're now setting. :-)

::: servo/components/style/properties/longhand/font.mako.rs:724
(Diff revision 3)
> +        if let SpecifiedValue::Keyword(kw, fraction)
> +                            = *specified_value {

Nit: maybe just on one line here?

::: servo/components/style/properties/properties.mako.rs:2154
(Diff revision 3)
>                  % if product == 'gecko':
>                      | LonghandId::TextOrientation
>                      | LonghandId::AnimationName
>                      | LonghandId::TransitionProperty
>                      | LonghandId::XLang
> +                    | LonghandId::MozScriptLevel

Do -moz-script-size-multiplier and -moz-script-size-min-size need to be in the early property set too?
Attachment #8858200 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8858199 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 15

2 months ago
mozreview-review
Comment on attachment 8859829 [details]
Bug 1355427 - Part 3: stylo: Update test expectations;

https://reviewboard.mozilla.org/r/131816/#review134696

Nice.
Attachment #8859829 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

2 months ago
mozreview-review
Comment on attachment 8859869 [details]
Bug 1355427 - Part 1: stylo: Support computing font-size against an arbitrary base size;

https://reviewboard.mozilla.org/r/131844/#review134704
Attachment #8859869 - Flags: review?(cam) → review+
(Assignee)

Comment 21

2 months ago
https://github.com/servo/servo/pull/16539

Comment 22

2 months ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1f1f3f34d558
Part 3: stylo: Update test expectations; r=heycam

Comment 23

2 months ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27507fdcbacd
Expectations adjustment followup. r=me

Comment 24

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f1f3f34d558
https://hg.mozilla.org/mozilla-central/rev/27507fdcbacd
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.