Closed
Bug 1367275
Opened 6 years ago
Closed 6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 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•6 years ago
|
||
https://github.com/servo/servo/pull/17058
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8870645 -
Attachment is obsolete: true
Comment 51•6 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•6 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•6 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: 6 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 59•6 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
•