Bug 1354970 - Add @counter-style rules #16455

SimonSapin · 2017-04-14T10:22:35Z

This change is

highfive · 2017-04-14T10:22:38Z

Heads up! This PR modifies the following files:

@bholley: components/style/lib.rs, components/style/values/mod.rs, components/style/stylist.rs, components/style/stylesheets.rs, components/style/properties/gecko.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/longhand/counters.mako.rs, components/style/gecko_bindings/sugar/ns_css_value.rs, components/style/counter_style.rs, components/style/properties/longhand/box.mako.rs, components/style/animation.rs, components/style/gecko/rules.rs, components/style/matching.rs
@KiChjang: components/script/dom/cssrule.rs, components/script/dom/csskeyframesrule.rs
@fitzgen: components/script/dom/cssrule.rs, components/script/dom/csskeyframesrule.rs
@emilio: components/style/lib.rs, components/style/values/mod.rs, components/style/stylist.rs, components/style/stylesheets.rs, components/style/properties/gecko.mako.rs, components/layout/generated_content.rs, components/style/properties/properties.mako.rs, components/style/properties/longhand/counters.mako.rs, components/style/gecko_bindings/sugar/ns_css_value.rs, components/style/counter_style.rs, components/style/properties/longhand/box.mako.rs, components/style/animation.rs, components/style/gecko/rules.rs, components/style/matching.rs

highfive · 2017-04-14T10:22:39Z

Warning

These commits modify style, layout, and script code, but no tests are modified. Please consider adding a test!

upsuper · 2017-04-15T02:14:45Z

IIRC, I'm supposed to review this pr? Assigning myself in case I forgot.

r? @upsuper

nox · 2017-04-16T11:16:41Z

@SimonSapin AFAICT you can squash "Fix up unit tests" with the CustomIdent commits.

bors-servo · 2017-04-17T10:44:10Z

☔ The latest upstream changes (presumably #16496) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2017-04-21T05:53:33Z

☔ The latest upstream changes (presumably #16553) made this pull request unmergeable. Please resolve the merge conflicts.

upsuper · 2017-04-21T06:42:59Z

Review status: 0 of 19 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

components/style/counter_style.rs, line 25 at r19 (raw file):

/// Parse the prelude of an @counter-style rule
pub fn parse_counter_style_name(input: &mut Parser) -> Result<CustomIdent, ()> {
    CustomIdent::from_ident(input.expect_ident()?, &["decimal", "none"])

This is wrong, actually. <counter-style-name> is <custom-ident> but it is lower-cased if it is a predefined counter style in the Counter Styles Level 3 spec. See the paragraph below the note in https://drafts.csswg.org/css-counter-styles/#typedef-counter-style-name

Comments from Reviewable

upsuper · 2017-04-21T06:43:47Z

(I comment that first because I suppose that may or may not affect the first commit in your patchset)

SimonSapin · 2017-04-24T08:57:01Z

Review status: 0 of 22 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

components/style/counter_style/mod.rs, line 25 at r19 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

This is wrong, actually. <counter-style-name> is <custom-ident> but it is lower-cased if it is a predefined counter style in the Counter Styles Level 3 spec. See the paragraph below the note in https://drafts.csswg.org/css-counter-styles/#typedef-counter-style-name

Fixed, and filed w3c/csswg-drafts#1285.

Comments from Reviewable

upsuper · 2017-04-24T15:27:53Z

Reviewed 1 of 2 files at r1, 18 of 19 files at r20, 8 of 8 files at r21, 3 of 3 files at r22, 3 of 3 files at r23, 1 of 1 files at r24, 5 of 6 files at r25, 3 of 4 files at r26, 2 of 3 files at r27, 1 of 2 files at r28, 2 of 3 files at r29, 2 of 3 files at r30, 3 of 4 files at r31, 2 of 3 files at r32, 1 of 2 files at r33, 2 of 3 files at r34, 2 of 3 files at r35, 2 of 3 files at r36, 1 of 2 files at r37, 2 of 3 files at r38, 2 of 2 files at r39, 2 of 2 files at r40, 3 of 3 files at r41.
Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

Cargo.lock, line 284 at r40 (raw file):

 "azure 0.15.0 (git+https://github.com/servo/rust-azure)",
 "canvas_traits 0.0.1",
 "cssparser 0.12.4 (registry+https://github.com/rust-lang/crates.io-index)",

Please update a Cargo.toml at the same time, otherwise Stylo build wouldn't get the update automatically.

components/script/dom/csskeyframesrule.rs, line 118 at r21 (raw file):

        // Setting this property to a CSS-wide keyword or `none` will
        // throw a Syntax Error.
        let name = CustomIdent::from_ident(value.into(), &["none"]).map_err(|()| Error::Syntax)?;

This is actually wrong. The name in keyframes rule can accept a string in CSS, thus it can accept arbitrary string. I think that also means there shouldn't be any check for CSSOM at all. But I guess that is probably a separate issue.

components/style/stylesheets.rs, line 531 at r21 (raw file):

pub struct KeyframesRule {
    /// The name of the current animation.
    pub name: CustomIdent,

Given that the name can be an arbitrary, it probably shouldn't be stored as a CustomIdent.

components/style/stylesheets.rs, line 940 at r21 (raw file):

    Viewport,
    /// A @keyframes rule, with its animation name and vendor prefix if exists.
    Keyframes(CustomIdent, Option<VendorPrefix>),

Ditto.

components/style/stylesheets.rs, line 1142 at r21 (raw file):

                let name = match input.next() {
                    Ok(Token::Ident(value)) => CustomIdent::from_ident(value, &["none"])?,
                    Ok(Token::QuotedString(value)) => CustomIdent(Atom::from(value)),

This is weird. Strings like "initial" is not really a valid custom ident. We probably need a new function in cssparser to serialize customident-or-string.

components/style/counter_style/update_predefined.py, line 11 at r41 (raw file):

        re.search(">([^>]+)</dfn>", line).group(1)
        for line in urllib.urlopen("https://drafts.csswg.org/css-counter-styles/")
        if 'data-dfn-for="<counter-style-name>"' in line

You missed Japanese and Chinese counter style types. This is probably an editorial issue of the spec, though, that they are using data-dfn-for="<counter-style>" rather than data-dfn-for="<counter-style-name>".

components/style/counter_style/update_predefined.py, line 17 at r41 (raw file):

        for name in names:
            # FIXME https://github.com/w3c/csswg-drafts/issues/1285
            if name == 'decimal':

This is probably wrong. You should include decimal and probably also none here, otherwise things like list-style-type: DeCiMaL may not work as expected.

components/style/properties/properties.mako.rs, line 845 at r40 (raw file):

        // FIXME(https://github.com/rust-lang/rust/issues/33156): remove this enum and use PropertyId
        // when stable Rust allows destructors in statics.
        pub enum StaticId {

This change seems to be unrelated.

components/style/counter_style.rs, line 20 at r26 (raw file):

/// Parse the prelude of an @counter-style rule
pub fn parse_counter_style_name(input: &mut Parser) -> Result<CustomIdent, ()> {
    CustomIdent::from_ident(input.expect_ident()?, &["decimal", "none"])

This is wrong. Nothing stops one from extending decimal at all. fallback (in a future commit) should accept decimal as well. The same applies to other usages of counter style name elsewhere, e.g. list-style-type, counter().

components/style/counter_style.rs, line 192 at r26 (raw file):

            }
            System::Extends(ref other) => {
                dest.write_str("symbolic ")?;

"extends "

components/style/counter_style.rs, line 155 at r31 (raw file):

    /// https://drafts.csswg.org/css-counter-styles/#counter-style-fallback
    "fallback" fallback / eCSSCounterDesc_Fallback: Fallback =
        Fallback(CustomIdent(Atom::from("decimal")));

Probably add a FIXME here for adding an atom for decimal.

components/style/counter_style.rs, line 239 at r32 (raw file):

}

impl OneOrMoreCommaSeparated for Symbol {}

It doesn't seem to me that <symbol> is ever used in comma separated list? I don't think this is necessary.

components/style/counter_style.rs, line 465 at r35 (raw file):

/// https://drafts.csswg.org/css-counter-styles/#descdef-counter-style-additive-symbols
#[derive(Debug, Clone)]
pub struct AdditiveSymbol {

It is called "additive tuple" in the spec. Probably this struct should be AdditiveTuple instead?

Comments from Reviewable

bors-servo · 2017-04-25T00:10:55Z

☔ The latest upstream changes (presumably #16589) made this pull request unmergeable. Please resolve the merge conflicts.

SimonSapin · 2017-04-25T08:55:57Z

Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

Cargo.lock, line 284 at r40 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

Please update a Cargo.toml at the same time, otherwise Stylo build wouldn't get the update automatically.

This will be moot when rebasing on top of #16589, though I’m holding off the rebase for now to make navigation in Reviewable easier. (I should have done the same for yesterday’s rebase but forgot.)

components/script/dom/csskeyframesrule.rs, line 118 at r21 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

This is actually wrong. The name in keyframes rule can accept a string in CSS, thus it can accept arbitrary string. I think that also means there shouldn't be any check for CSSOM at all. But I guess that is probably a separate issue.

Fixed.

components/style/stylesheets.rs, line 531 at r21 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

Given that the name can be an arbitrary, it probably shouldn't be stored as a CustomIdent.

Added a dedicated KeyframesName type, for <keyframes-name> in the spec.

components/style/stylesheets.rs, line 940 at r21 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

Ditto.

Changed to KeyframesName.

components/style/stylesheets.rs, line 1142 at r21 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

This is weird. Strings like "initial" is not really a valid custom ident. We probably need a new function in cssparser to serialize customident-or-string.

Fixed with KeyframesName.

components/style/counter_style/mod.rs, line 20 at r26 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

This is wrong. Nothing stops one from extending decimal at all. fallback (in a future commit) should accept decimal as well. The same applies to other usages of counter style name elsewhere, e.g. list-style-type, counter().

Changed parse_counter_style_name to allow decimal and none (the former lower-cased) and @counter-style parsing to exclude them.

components/style/counter_style/mod.rs, line 192 at r26 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

"extends "

Fixed.

components/style/counter_style/mod.rs, line 155 at r31 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

Probably add a FIXME here for adding an atom for decimal.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1359323 and added a FIXME linking to it.

components/style/counter_style/mod.rs, line 239 at r32 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

It doesn't seem to me that <symbol> is ever used in comma separated list? I don't think this is necessary.

Left over from before I realized symbols does not use commas. Removed.

components/style/counter_style/mod.rs, line 465 at r35 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

It is called "additive tuple" in the spec. Probably this struct should be AdditiveTuple instead?

Done.

components/style/counter_style/update_predefined.py, line 11 at r41 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

You missed Japanese and Chinese counter style types. This is probably an editorial issue of the spec, though, that they are using data-dfn-for="<counter-style>" rather than data-dfn-for="<counter-style-name>".

Fixed.

components/style/counter_style/update_predefined.py, line 17 at r41 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

This is probably wrong. You should include decimal and probably also none here, otherwise things like list-style-type: DeCiMaL may not work as expected.

Ok for decimal, but why none? list-style-type: none will be parsed explicitly, not through this code.

components/style/properties/properties.mako.rs, line 845 at r40 (raw file):

Previously, upsuper (Xidorn Quan) wrote…

This change seems to be unrelated.

It is due to the cssparser update. The split into multiple commits is significant :)

This fixes a "private in public" warning caused by ascii_case_insensitive_phf_map! now generating a public function.

Comments from Reviewable

bors-servo · 2017-04-26T05:38:50Z

⌛ Testing commit f881bc7 with merge 0bd3306...

Bug 1354970 - Add @counter-style rules  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16455)

bors-servo · 2017-04-26T05:41:09Z

💔 Test failed - mac-dev-unit

w3c/csswg-drafts@063dc05 web-platform-tests/wpt#5695

SimonSapin · 2017-04-26T05:55:15Z

Manifest update.

@bors-servo r=upsuper

bors-servo · 2017-04-26T05:55:16Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors-servo · 2017-04-26T05:55:16Z

📌 Commit f881bc7 has been approved by upsuper

bors-servo · 2017-04-26T05:55:18Z

⚡ Previous build results for mac-rel-wpt1, windows-msvc-dev are reusable. Rebuilding only android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt2...

bors-servo · 2017-04-26T05:56:33Z

💔 Test failed - arm32

SimonSapin · 2017-04-26T05:59:55Z

Failed to push…

@bors-servo r=upsuper

bors-servo · 2017-04-26T05:59:56Z

📌 Commit 1b41900 has been approved by upsuper

bors-servo · 2017-04-26T06:00:00Z

⌛ Testing commit 1b41900 with merge 2eff661...

Bug 1354970 - Add @counter-style rules  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16455)

bors-servo · 2017-04-26T06:35:31Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: upsuper
Pushing 2eff661 to master...

highfive assigned glennw Apr 14, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 14, 2017

SimonSapin force-pushed the counter-style branch from ad32c29 to 50ceeff Compare April 14, 2017 13:29

SimonSapin changed the title ~~Add @counter-style rules~~ Bug 1354970 - Add @counter-style rules Apr 14, 2017

highfive assigned upsuper and unassigned glennw Apr 15, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Apr 17, 2017

SimonSapin force-pushed the counter-style branch from 50ceeff to 10ba982 Compare April 19, 2017 03:14

upsuper added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 24, 2017

SimonSapin force-pushed the counter-style branch from 10ba982 to 1861809 Compare April 24, 2017 08:50

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 24, 2017

upsuper added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 24, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Apr 25, 2017

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 25, 2017

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 26, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 26, 2017

CSSKeyframesRule::name setter doesn’t throw anymore

1b41900

w3c/csswg-drafts@063dc05 web-platform-tests/wpt#5695

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 26, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 26, 2017

SimonSapin force-pushed the counter-style branch from f881bc7 to 1b41900 Compare April 26, 2017 05:59

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 26, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 26, 2017

bors-servo merged commit 1b41900 into master Apr 26, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 26, 2017

bors-servo mentioned this pull request Apr 26, 2017

Implement unitless length quirk #16609

Merged

SimonSapin deleted the counter-style branch April 26, 2017 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1354970 - Add @counter-style rules #16455

Bug 1354970 - Add @counter-style rules #16455

SimonSapin commented Apr 14, 2017 •

edited by larsbergstrom

Loading

highfive commented Apr 14, 2017

highfive commented Apr 14, 2017

upsuper commented Apr 15, 2017

nox commented Apr 16, 2017

bors-servo commented Apr 17, 2017

bors-servo commented Apr 21, 2017

upsuper commented Apr 21, 2017

upsuper commented Apr 21, 2017

SimonSapin commented Apr 24, 2017

upsuper commented Apr 24, 2017

bors-servo commented Apr 25, 2017

SimonSapin commented Apr 25, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

SimonSapin commented Apr 26, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

SimonSapin commented Apr 26, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

Bug 1354970 - Add @counter-style rules #16455

Bug 1354970 - Add @counter-style rules #16455

Conversation

SimonSapin commented Apr 14, 2017 • edited by larsbergstrom Loading

highfive commented Apr 14, 2017

highfive commented Apr 14, 2017

upsuper commented Apr 15, 2017

nox commented Apr 16, 2017

bors-servo commented Apr 17, 2017

bors-servo commented Apr 21, 2017

upsuper commented Apr 21, 2017

upsuper commented Apr 21, 2017

SimonSapin commented Apr 24, 2017

upsuper commented Apr 24, 2017

bors-servo commented Apr 25, 2017

SimonSapin commented Apr 25, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

SimonSapin commented Apr 26, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

SimonSapin commented Apr 26, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

bors-servo commented Apr 26, 2017

SimonSapin commented Apr 14, 2017 •

edited by larsbergstrom

Loading