Closed
Bug 1367275
Opened 8 years ago
Closed 8 years ago
stylo: Support remaining longhands
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: manishearth, Assigned: manishearth)
References
Details
Attachments
(5 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
Only a couple left. Support them.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a1e50acf6e963232316c85b4092846fad107727
With these fixed we'll only have the grid and grid-template shorthands.
Comment 5•8 years ago
|
||
mozreview-review |
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 6•8 years ago
|
||
mozreview-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.
Assignee | ||
Comment 7•8 years ago
|
||
mozreview-review |
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 8•8 years ago
|
||
mozreview-review |
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 9•8 years ago
|
||
mozreview-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)
Assignee | ||
Comment 11•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•8 years ago
|
||
mozreview-review-reply |
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 19•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 20•8 years ago
|
||
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.
Comment 21•8 years ago
|
||
mozreview-review |
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 22•8 years ago
|
||
mozreview-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+
Assignee | ||
Comment 23•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•8 years ago
|
||
mozreview-review-reply |
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.
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) |
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 | ||
Comment 46•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8870645 -
Attachment is obsolete: true
Comment 51•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 57•8 years ago
|
||
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 58•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/48e940ef1476
https://hg.mozilla.org/mozilla-central/rev/3b0e4a93bdee
https://hg.mozilla.org/mozilla-central/rev/79aa3203ab9e
https://hg.mozilla.org/mozilla-central/rev/72204f9be784
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 59•8 years ago
|
||
mozreview-review |
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.
Description
•