Closed Bug 1339711 Opened 7 years ago Closed 7 years ago

stylo: Handle background and mathml presentation attributes

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: manishearth, Assigned: manishearth)

References

Details

Attachments

(5 files, 1 obsolete file)

Bug 1339711 - Part 1: stylo: Support -moz-script-size-multiplier, -moz-script-level, -moz-math-display;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1339711 - Part 2: stylo: Support most MathML presentation attributes;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1339711 - Part 3: stylo: Support mathsize, width, and scriptminsize presentation attributes in MathML;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1339711 - Part 4: stylo: Update reftest expectations for MathML changes;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1339711 - Part 5: stylo: Support background presentation attribute;
59 bytes, text/x-review-board-request
emilio
: review+
Details 
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.
Priority: -- → P3
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)
Blocks: 1351973
There are a lot of mathml reftests, so I think we should bump the priority of this one.
Priority: P3 → P1
Assignee: nobody → manishearth
Blocks: stylo
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 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 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 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 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)
> 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.
Attachment #8856850 - Attachment is obsolete: true
Attachment #8856850 - Flags: review?(emilio+bugs)
Blocks: 1355427
Filed bug 1355427 to deal with the remaining mathml failures.
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 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 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+
> 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)
Added support for background
Depends on: 1310885
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+
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).
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.

Attachment

General

Creator:
Created:
Updated:
Size: