Closed Bug 1367275 Opened 6 years ago Closed 6 years ago

stylo: Support remaining longhands

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

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 1367275 - Part 2: stylo: Support -moz-window-shadow;
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1367275 - Part 3: stylo: Support font-variation-settings;
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1367275 - Part 4: stylo: Support -moz-context-properties;
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1367275 - Part 5: stylo: Support -moz-min-font-size-ratio;
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1367275 - Part 1: stylo: Support column-span;
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
Only a couple left. Support them.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a1e50acf6e963232316c85b4092846fad107727

With these fixed we'll only have the grid and grid-template shorthands.
Comment on attachment 8870645 [details]
Bug 1367275 - Part 1: stylo: Support column-span;

https://reviewboard.mozilla.org/r/142110/#review145764
Attachment #8870645 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8870645 [details]
Bug 1367275 - Part 1: stylo: Support column-span;

https://reviewboard.mozilla.org/r/142110/#review145766

::: servo/components/style/properties/longhand/column.mako.rs:57
(Diff revision 1)
>                            initial_specified_value="specified::CSSColor::currentcolor()",
>                            products="gecko", animation_value_type="IntermediateColor", extra_prefixes="moz",
>                            complex_color=True, need_clone=True,
>                            spec="https://drafts.csswg.org/css-multicol/#propdef-column-rule-color")}
>  
>  // It's not implemented in servo or gecko yet.

Removing "or gecko"?

::: servo/components/style/properties/longhand/column.mako.rs:58
(Diff revision 1)
>                            products="gecko", animation_value_type="IntermediateColor", extra_prefixes="moz",
>                            complex_color=True, need_clone=True,
>                            spec="https://drafts.csswg.org/css-multicol/#propdef-column-rule-color")}
>  
>  // It's not implemented in servo or gecko yet.
>  ${helpers.single_keyword("column-span", "none all",

This is still behind pref "layout.css.column-span.enabled" which hasn't been enabled yet, so it shouldn't be enabled unconditionally here.
Comment on attachment 8870645 [details]
Bug 1367275 - Part 1: stylo: Support column-span;

https://reviewboard.mozilla.org/r/142110/#review145772

::: servo/components/style/properties/longhand/column.mako.rs:58
(Diff revision 1)
>                            products="gecko", animation_value_type="IntermediateColor", extra_prefixes="moz",
>                            complex_color=True, need_clone=True,
>                            spec="https://drafts.csswg.org/css-multicol/#propdef-column-rule-color")}
>  
>  // It's not implemented in servo or gecko yet.
>  ${helpers.single_keyword("column-span", "none all",

That's not an issue, property prefs are already supported. There's machinery that will figure out that this is pref-enabled and check the pref in that case.
Comment on attachment 8870646 [details]
Bug 1367275 - Part 2: stylo: Support -moz-window-shadow;

https://reviewboard.mozilla.org/r/142112/#review145790

I'm not a big fan of adding more headers into binding files... but I guess this is fine...

If I were you, I may try to move those constants to nsStyleConsts.h instead, but I guess that's not necessary.

::: servo/components/style/properties/longhand/ui.mako.rs:40
(Diff revision 1)
> +
> +

Do we need two empty lines here?
Attachment #8870646 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8870647 [details]
Bug 1367275 - Part 3: stylo: Support font-variation-settings;

https://reviewboard.mozilla.org/r/142114/#review145794

::: layout/style/ServoBindings.toml:259
(Diff revision 1)
> -    "mozilla::gfx::.*",
> +    # We want everything but FontVariation to be opaque but we don't have negative regexes ;_;
> +    "mozilla::gfx::(.{0,12}|.{14,}|([^F][^o][^n][^t][^V][^a][^r][^i][^a][^t][^i][^o][^n]))",

This is... smart.. but not quite scalable...

Why were `mozilla::gfx::*` all excluded? It seems this happend in servo/servo#15216 for updating bindgen without further description on what's going wrong. Could we replace this line with a list of problematic types instead?

::: servo/components/style/properties/gecko.mako.rs:1532
(Diff revision 1)
> +
> +        let current_settings = &mut self.gecko.mFont.fontVariationSettings;
> +        current_settings.clear_pod();
> +
> +        match v {
> +            FontSettings::Normal => unsafe { current_settings.set_len_pod(0) },

Looks unnecessary. `clear_pod` already sets the length to zero IIUC, right?

::: servo/components/style/properties/longhand/font.mako.rs:1816
(Diff revision 1)
> -            /// <string> [ on | off | <integer> ]
> -            fn parse(_context: &ParserContext, input: &mut Parser) -> Result<Self, ()> {
> -                use std::io::Cursor;
> -                use byteorder::{ReadBytesExt, BigEndian};
>  
> -                let tag = try!(input.expect_string());
> +    pub type SpecifiedValue = computed_value::T;

`font-variation-settings` should be a subprop of `font` shorthand, so it needs the system font bits.

::: servo/components/style/values/generics/mod.rs:110
(Diff revision 1)
> +#[derive(Clone, Debug, Eq, PartialEq)]
> +#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
> +/// A settings tag, defined by a four-character tag and a setting value
> +///
> +/// For font-feature-settings, this is a tag and an integer,
> +/// for font-variation-settings this is a tag and a float

I'm not completely sure but it seems to me we generally put doc before attributes?

::: servo/components/style/values/generics/mod.rs:142
(Diff revision 1)
> +                let mut iter = ftvs.iter();
> +                // handle head element
> +                try!(iter.next().unwrap().to_css(dest));
> +                // handle tail, precede each with a delimiter
> +                for ftv in iter {
> +                    try!(dest.write_str(", "));
> +                    try!(ftv.to_css(dest));
> +                }
> +                Ok(())

You can add
```rust
impl<T> OneOrMoreCommaSeparated for FontSettingTag<T> {}
```
then you can just use `ftvs.to_css(dest)` here, and `Vec::FontSettingTag::parse(context, input)` below.

::: servo/components/style/values/generics/mod.rs:251
(Diff revision 1)
> +#[derive(Clone, Debug, Eq, PartialEq)]
> +#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
> +/// A font settings value for font-variation-settings or font-feature-settings

Order between doc and attributes here, too.

::: servo/components/style/values/generics/mod.rs:254
(Diff revision 1)
> +pub enum FontSettings<T> {
> +    /// No settings (default)
> +    Normal,
> +    /// Set of settings
> +    Tag(Vec<FontSettingTag<T>>)
> +}

Also it seems to be a bit confusing to have the impl block of this type far before the declaration of the type itself... Consider moving those down?
Attachment #8870647 - Flags: review?(xidorn+moz)
Comment on attachment 8870647 [details]
Bug 1367275 - Part 3: stylo: Support font-variation-settings;

https://reviewboard.mozilla.org/r/142114/#review145794

> This is... smart.. but not quite scalable...
> 
> Why were `mozilla::gfx::*` all excluded? It seems this happend in servo/servo#15216 for updating bindgen without further description on what's going wrong. Could we replace this line with a list of problematic types instead?

It contains a ton of problematic types. I tried specifically blacklisting those but it didn't help.

We rarely need things from there, so I'm fine with this for now. Emilio is looking into making this no longer a problem I think.

> `font-variation-settings` should be a subprop of `font` shorthand, so it needs the system font bits.

It's not a subproperty.

https://searchfox.org/mozilla-central/source/layout/style/nsCSSParser.cpp#14096
Comment on attachment 8870647 [details]
Bug 1367275 - Part 3: stylo: Support font-variation-settings;

https://reviewboard.mozilla.org/r/142114/#review145794

> It's not a subproperty.
> 
> https://searchfox.org/mozilla-central/source/layout/style/nsCSSParser.cpp#14096

OK, I think Gecko is wrong, then, but we can fix it later.
Comment on attachment 8870647 [details]
Bug 1367275 - Part 3: stylo: Support font-variation-settings;

https://reviewboard.mozilla.org/r/142114/#review146226
Attachment #8870647 - Flags: review?(xidorn+moz) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&author=manishearth@gmail.com&selectedJob=101761899

Makes one test pass; but also causes some crashes due to bug 1362599 not handling xml:lang. I'll go fix that first then.
Depends on: 1367619
Comment on attachment 8870954 [details]
Bug 1367275 - Part 4: stylo: Support -moz-context-properties;

https://reviewboard.mozilla.org/r/142510/#review146228

r=me with issues addressed.

::: servo/components/style/gecko_bindings/sugar/ns_com_ptr.rs:17
(Diff revision 1)
> +    /// Set this pointer to a raw pointer.
> +    #[inline]
> +    pub fn set_raw<U>(&mut self, ptr: *mut T)  {
> +        self.mRawPtr = ptr;
> +    }

I'm adding a similiar function in servo/servo#17016 called `set_raw_from_addrefed`. You should probably wait for that to merge and use it.

Actually it was named `set_raw` initially, and heycam suggested that the name wasn't clear in bug 1366735 comment 6, so I changed.

::: servo/components/style/properties/gecko.mako.rs:4104
(Diff revision 1)
> +        unsafe {
> +            bindings::Gecko_nsStyleSVG_SetContextPropertiesLength(&mut self.gecko, v.len() as u32);
> +        }

Hmmm, `mContextProps` should probably use `nsCOMArray` instead so that we can share code between this and `mWillChange`. Or alternatively change `mWillChange` to use `nsTArray<nsCOMPtr<nsIAtom>>` which may actually be easier and more efficient to handle given we can handle it purely in Rust code.

(I have a feeling that we can probably treat `nsTArray<nsCOMPtr<_>>` as a POD if we end up filling every slot in Rust code.)

I'm also fine with the current code and we can do the code sharing in a followup bug.

::: servo/components/style/properties/gecko.mako.rs:4110
(Diff revision 1)
> +            bindings::Gecko_nsStyleSVG_SetContextPropertiesLength(&mut self.gecko, v.len() as u32);
> +        }
> +
> +        self.gecko.mContextPropsBits = 0;
> +        for (mut gecko, servo) in self.gecko.mContextProps.iter_mut().zip(v) {
> +            if servo.0 == atom!("stroke") {

`atom!("fill")`

::: servo/components/style/properties/gecko.mako.rs:4110
(Diff revision 1)
> +            if servo.0 == atom!("stroke") {
> +                self.gecko.mContextPropsBits |= structs::NS_STYLE_CONTEXT_PROPERTY_FILL as u8;
> +            } else if servo.0 == atom!("stroke") {
> +                self.gecko.mContextPropsBits |= structs::NS_STYLE_CONTEXT_PROPERTY_STROKE as u8;
> +            }

Maybe more compact to do
```rust
self.gecko.mContextPropsBits |= if servo.0 == atom!("fill") {
    structs::NS_STYLE_CONTEXT_PROPERTY_FILL as u8
} else if ...
```
but either way is fine.

::: servo/components/style/properties/longhand/inherited_svg.mako.rs:294
(Diff revision 1)
> +    }
> +
> +
> +    pub fn parse(_context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
> +        let i = input.expect_ident()?;
> +        CustomIdent::from_ident(i, &["all", "none", "auto"])

It seems this should be `&["none", "default"]` according to the Gecko parser code. https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/layout/style/nsCSSParser.cpp#8078-8091
Attachment #8870954 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8871031 [details]
Bug 1367275 - Part 5: stylo: Support -moz-min-font-size-ratio;

https://reviewboard.mozilla.org/r/142588/#review146244

font-size seems to be pretty complicated... I don't fully follow how it is handled, but the changes look reasonable to me. r=me with issues addressed.

::: servo/components/style/properties/gecko.mako.rs:1907
(Diff revision 2)
> +        let percentage = if v.0 > 255. {
> +            255.
> +        } else if v.0 < 0. {
> +            0.
> +        } else {
> +            v.0
> +        };

Probably worth copying the comment in nsRuleNode here as well, or we can move that comment to `nsStyleStruct.h` somehow instead.

::: servo/components/style/properties/longhand/font.mako.rs:967
(Diff revision 2)
> +        style.mutate_font()
> -               .set_font_size(computed);
> +             .set_font_size(computed);

Probably merge the two lines?

::: servo/components/style/properties/longhand/font.mako.rs:2395
(Diff revision 2)
> +${helpers.predefined_type("-moz-min-font-size-ratio",
> +                          "Percentage",
> +                          "computed::Percentage::hundred()",
> +                          animation_value_type="none",
> +                          products="gecko",
> +                          spec="Nonstandard")}

And this is an internal one which shouldn't be exposed to the web.

::: servo/components/style/properties/properties.mako.rs:2650
(Diff revision 2)
>              // Font size must be explicitly inherited to handle lang changes and
>              // scriptlevel changes.
>              } else if seen.contains(LonghandId::XLang) ||
>                        seen.contains(LonghandId::MozScriptLevel) ||
>                        font_family.is_some() {

And `-moz-min-font-size-ratio` change as well, I suppose?
Attachment #8871031 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8870954 [details]
Bug 1367275 - Part 4: stylo: Support -moz-context-properties;

https://reviewboard.mozilla.org/r/142510/#review146228

> It seems this should be `&["none", "default"]` according to the Gecko parser code. https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/layout/style/nsCSSParser.cpp#8078-8091

Not according to the test

https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/layout/style/test/property_database.js#7957

I assume "all" gets parsed as eCSSUnit_All or something.
Comment on attachment 8870954 [details]
Bug 1367275 - Part 4: stylo: Support -moz-context-properties;

https://reviewboard.mozilla.org/r/142510/#review146228

> Not according to the test
> 
> https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/layout/style/test/property_database.js#7957
> 
> I assume "all" gets parsed as eCSSUnit_All or something.

That shouldn't happen... The parsing code doesn't include `VARIANT_ALL` so `ParseSingleTokenVariant` shouldn't produce `eCSSUnit_All` value. I think the best explanation is that, since this test is not running, the Gecko code may not really pass it.

I guess it doesn't matter a lot given it's internal.
Attachment #8870645 - Attachment is obsolete: true
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d4bcd6e4f4a7 -d e156625e27c9: rebasing 398642:d4bcd6e4f4a7 "Bug 1367275 - Part 2: stylo: Support -moz-window-shadow; r=xidorn"
rebasing 398643:1167a74d7bfc "Bug 1367275 - Part 3: stylo: Support font-variation-settings; r=xidorn"
merging layout/style/ServoBindings.toml
rebasing 398644:286fbf9b186d "Bug 1367275 - Part 4: stylo: Support -moz-context-properties; r=xidorn"
merging layout/style/ServoBindings.cpp
merging layout/style/ServoBindings.h
rebasing 398645:98d8ec943c14 "Bug 1367275 - Part 5: stylo: Support -moz-min-font-size-ratio; r=xidorn" (tip)
merging layout/reftests/mathml/reftest.list
merging layout/style/ServoBindings.cpp
merging layout/style/ServoBindings.h
warning: conflicts while merging layout/reftests/mathml/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/48e940ef1476
Part 2: stylo: Support -moz-window-shadow; r=xidorn
https://hg.mozilla.org/integration/autoland/rev/3b0e4a93bdee
Part 3: stylo: Support font-variation-settings; r=xidorn
https://hg.mozilla.org/integration/autoland/rev/79aa3203ab9e
Part 4: stylo: Support -moz-context-properties; r=xidorn
https://hg.mozilla.org/integration/autoland/rev/72204f9be784
Part 5: stylo: Support -moz-min-font-size-ratio; r=xidorn
Comment on attachment 8872091 [details]
Bug 1367275 - Part 1: stylo: Support column-span;

https://reviewboard.mozilla.org/r/143590/#review147368
Attachment #8872091 - Flags: review?(xidorn+moz) → review+
You need to log in before you can comment on or make changes to this bug.