stylo: Handle background and mathml presentation attributes

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
11 months ago
9 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

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

11 months ago
Bug 1338936 adds support for non-svg presentation attributes in stylo, except for the ones used in mathml (script-size-multiplier, etc). We should handle these.

Updated

10 months ago
Priority: -- → P3
(Assignee)

Comment 1

10 months ago
For background, there's this weird loading behavior where the image is sometimes loaded off the attribute and sometimes made into a style rule. I'm not sure what we should do here. Also, that const_cast gives me thread safety heebie jeebies :)

http://searchfox.org/mozilla-central/source/dom/html/nsGenericHTMLElement.cpp#1533

Any idea what we should be doing?
Flags: needinfo?(cam)

Updated

10 months ago
Blocks: 1351973

Comment 2

10 months ago
There are a lot of mathml reftests, so I think we should bump the priority of this one.
Priority: P3 → P1

Updated

10 months ago
Assignee: nobody → manishearth
(Assignee)

Updated

9 months ago
Blocks: 1243581
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

9 months ago
So far I'm only handling the mathml stuff, not background. Will get to that eventually.

This adds the scriptminsize property, but does not fix the dependence of fontsize on that; we can do that later I guess.

Not yet tested, pushing patches up early while I wait for try.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

9 months ago
mozreview-review
Comment on attachment 8856848 [details]
Bug 1339711 - Part 1: stylo: Support -moz-script-size-multiplier, -moz-script-level, -moz-math-display;

https://reviewboard.mozilla.org/r/128768/#review131298

::: servo/components/style/properties/longhand/font.mako.rs:1220
(Diff revision 2)
> +                    let parent = cx.inherited_style().get_font().clone__moz_script_level();
> +                    parent as i32 + rel
> +                }
> +                SpecifiedValue::Absolute(abs) => abs,
> +            };
> +            if int > i8::MAX as i32 {

just use cmp::max and cmp::min?

::: servo/components/style/properties/longhand/font.mako.rs:1233
(Diff revision 2)
> +        }
> +    }
> +
> +    pub fn parse(_context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
> +        if let Ok(i) = input.try(|i| i.expect_integer()) {
> +            Ok(SpecifiedValue::Relative(i))

return here, deindent the rest.
Attachment #8856848 - Flags: review?(emilio+bugs) → review+

Comment 13

9 months ago
mozreview-review
Comment on attachment 8856850 [details]
Bug 1339711 - Part 3: stylo: Add SpecifiedLengthUnit type;

https://reviewboard.mozilla.org/r/128772/#review131304

This intersects a lot with https://github.com/servo/servo/pull/16229. That's about to land, and I think we can implement this without the new enum, what do you think?
Attachment #8856850 - Flags: review?(emilio+bugs)

Comment 14

9 months ago
mozreview-review
Comment on attachment 8856851 [details]
Bug 1339711 - Part 3: stylo: Support mathsize, width, and scriptminsize presentation attributes in MathML;

https://reviewboard.mozilla.org/r/128774/#review131306

Clearing waiting for the issue in the previous patch to be discussed.

::: dom/mathml/nsMathMLElement.cpp:641
(Diff revision 2)
>            if (str.EqualsASCII(sizes[i])) {
> -            fontSize->SetIntValue(values[i], eCSSUnit_Enumerated);
> +            aData->SetKeywordValue(eCSSProperty_font_size, values[i]);
>              break;
>            }
>          }
> +      } else if (fontSize.GetUnit() != eCSSUnit_Null) {

nit:

```
} else if (fontSize.GetUnit() == eCSSUnit_Percent) {
    // ...
} else if (fontSize.GetUnit() == eCSSUnit_Null) {
    // ...
}
```

::: dom/mathml/nsMathMLElement.cpp:647
(Diff revision 2)
> +        if (fontSize.GetUnit() == eCSSUnit_Percent) {
> +          aData->SetPercentValue(eCSSProperty_font_size,
> +                                 fontSize.GetPercentValue());
> +        } else {
> +          aData->SetLengthValue(eCSSProperty_font_size, fontSize); 
> +        }

nit: Whitespace
Attachment #8856851 - Flags: review?(emilio+bugs)

Comment 15

9 months ago
mozreview-review
Comment on attachment 8856849 [details]
Bug 1339711 - Part 2: stylo: Support most MathML presentation attributes;

https://reviewboard.mozilla.org/r/128770/#review131302

::: dom/mathml/nsMathMLElement.cpp:665
(Diff revision 2)
>                       aData->mPresContext->Document());
>      }
>      if (value && value->Type() == nsAttrValue::eString &&
> -        fontFamily->GetUnit() == eCSSUnit_Null) {
> +        !aData->PropertyIsSet(eCSSProperty_font_family)) {
> +      aData->SetFontFamily(value->GetStringValue());
>        nsCSSParser parser;

This variable is unused now.

::: servo/components/style/properties/longhand/font.mako.rs:311
(Diff revision 2)
>                  _ => Err(())
>              }
>          })
>      }
>  
> +    impl SpecifiedValue {

This intersects a lot with the code right above it, let's reuse it.
Attachment #8856849 - Flags: review?(emilio+bugs)
(Assignee)

Comment 16

9 months ago
> This intersects a lot with https://github.com/servo/servo/pull/16229. That's about to land, and I think we can implement this without the new enum, what do you think?

works for me.
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)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8856850 - Attachment is obsolete: true
Attachment #8856850 - Flags: review?(emilio+bugs)
(Assignee)

Updated

9 months ago
Blocks: 1355427
(Assignee)

Comment 26

9 months ago
Filed bug 1355427 to deal with the remaining mathml failures.

Comment 27

9 months ago
mozreview-review
Comment on attachment 8856849 [details]
Bug 1339711 - Part 2: stylo: Support most MathML presentation attributes;

https://reviewboard.mozilla.org/r/128770/#review131692

::: servo/components/style/properties/longhand/font.mako.rs:312
(Diff revision 4)
> -        })
> -    }
> +        }
>  
> +        pub fn from_gecko_keyword(kw: u32) -> Self {
> +            Self::from_int(kw as i32).expect("Found unexpected value in style
> +                                       struct for font-weight property")

Alignment looks odd here, please fix it.
Attachment #8856849 - Flags: review?(emilio+bugs) → review+

Comment 28

9 months ago
mozreview-review
Comment on attachment 8856851 [details]
Bug 1339711 - Part 3: stylo: Support mathsize, width, and scriptminsize presentation attributes in MathML;

https://reviewboard.mozilla.org/r/128774/#review131694

::: dom/mathml/nsMathMLElement.cpp:861
(Diff revision 4)
>        // This does not handle auto and unitless values
>        if (value && value->Type() == nsAttrValue::eString) {
> -        ParseNumericValue(value->GetStringValue(), *width, 0,
> +        ParseNumericValue(value->GetStringValue(), width, 0,
>                            aData->mPresContext->Document());
> +        if (width.GetUnit() != eCSSUnit_Null) {
> +          if (width.GetUnit() == eCSSUnit_Percent) {

Maybe:

```
if (width.GetUnit() == Percent) {
    // ...
} else if (width.GetUnit() != Null) {
    // ...
}
```
Attachment #8856851 - Flags: review?(emilio+bugs) → review+

Comment 29

9 months ago
mozreview-review
Comment on attachment 8856910 [details]
Bug 1339711 - Part 4: stylo: Update reftest expectations for MathML changes;

https://reviewboard.mozilla.org/r/128834/#review131696

Do you know why the rest of the failing ones are? It'd be nice to file bugs and annotate them.
Attachment #8856910 - Flags: review?(emilio+bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

9 months ago
> Do you know why the rest of the failing ones are? It'd be nice to file bugs and annotate them.

Comment 26:

> Filed bug 1355427 to deal with the remaining mathml failures.

It's all scriptlevel stuff. The tests seem to mostly be fractions and super/subscripts, which use -moz-script-level to tweak the font-size. This PR adds support for -moz-script-level but does not handle that part (which IMO is out of scope).



-----------

Oh, also, for background, heycam said that we should just serialize and reparse for now, making it possible to cache gecko image requests in servo specified values is under the scope of bug 1310885. I'll add a commit doing that.
Flags: needinfo?(cam)
Comment hidden (mozreview-request)
(Assignee)

Comment 36

9 months ago
Added support for background
Depends on: 1310885
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

9 months ago
mozreview-review
Comment on attachment 8857286 [details]
Bug 1339711 - Part 5: stylo: Support background presentation attribute;

https://reviewboard.mozilla.org/r/129234/#review131894

r=me with a comment/bug filled for the asserts if not pre-existing.

::: layout/reftests/bugs/reftest-stylo.list:1586
(Diff revision 2)
>  == 581579-1.html 581579-1.html
>  == 582037-1a.html 582037-1a.html
>  == 582037-1b.html 582037-1b.html
>  fuzzy-if(Android,3,256) == 582037-2a.html 582037-2a.html
>  fuzzy-if(Android,3,256) == 582037-2b.html 582037-2b.html
> -asserts(1-2) == 582146-1.html 582146-1.html
> +asserts(1-3) == 582146-1.html 582146-1.html

Please leave a comment with which assertion it is, or a bug reference, or something.

::: layout/style/ServoSpecifiedValues.h:107
(Diff revision 2)
>      }
>    }
>  
>    void SetFontFamily(const nsString& aValue);
>    void SetTextDecorationColorOverride();
> +  void SetBackgroundImage(nsAttrValue& aValue);

I think we can make this `const nsAttrValue&` instead.

::: layout/style/ServoSpecifiedValues.cpp:127
(Diff revision 2)
> +{
> +  // FIXME (bug 1310885) this should store resolved images directly on the servo
> +  // side.
> +  nsAutoString str;
> +  aValue.ToString(str);
> +  Servo_DeclarationBlock_SetBackgroundImage(mDecl, str, mPresContext->Document()->DefaultStyleAttrURLData());

Please wrap to 80 lines.

::: layout/style/nsRuleData.cpp:71
(Diff revision 2)
> +{
> +  nsCSSValue* backImage = ValueForBackgroundImage();
> +  // If the value is an image, or it is a URL and we attempted a load,
> +  // put it in the style tree.
> +  if (aValue.Type() == nsAttrValue::eURL) {
> +    aValue.LoadImage(mPresContext->Document());

Oh, I guess we can't because of this, disregard this then.

::: servo/ports/geckolib/glue.rs:1555
(Diff revision 2)
> +    use style::values::specified::url::SpecifiedUrl;
> +
> +    let url_data = unsafe { RefPtr::from_ptr_ref(&raw_extra_data) };
> +    let string = unsafe { (*value).to_string() };
> +    let error_reporter = StdoutErrorReporter;
> +    let context = ParserContext::new(Origin::Author, url_data, &error_reporter);

We should really really consider doing something to reduce the boilerplate here.
Attachment #8857286 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 40

9 months ago
mozreview-review-reply
Comment on attachment 8857286 [details]
Bug 1339711 - Part 5: stylo: Support background presentation attribute;

https://reviewboard.mozilla.org/r/129234/#review131894

> Please leave a comment with which assertion it is, or a bug reference, or something.

Turns out this assertion was a leftover one from the absolute values fix (which I rebased over).
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 47

9 months ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/70023ccd2733
Part 1: stylo: Support -moz-script-size-multiplier, -moz-script-level, -moz-math-display; r=emilio
https://hg.mozilla.org/integration/autoland/rev/888ef3f2eb92
Part 2: stylo: Support most MathML presentation attributes; r=emilio
https://hg.mozilla.org/integration/autoland/rev/5183bd9c0a52
Part 3: stylo: Support mathsize, width, and scriptminsize presentation attributes in MathML; r=emilio
https://hg.mozilla.org/integration/autoland/rev/b7a7fec82a7c
Part 4: stylo: Update reftest expectations for MathML changes; r=emilio
https://hg.mozilla.org/integration/autoland/rev/b118d3c85081
Part 5: stylo: Support background presentation attribute; r=emilio
You need to log in before you can comment on or make changes to this bug.