Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support property aliases #14893

Merged
merged 1 commit into from Jan 7, 2017
Merged

Support property aliases #14893

merged 1 commit into from Jan 7, 2017

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Jan 6, 2017

Aliases now get forwarded to their "original" property name at parse time. CSSOM continues to provide access through alias getters.

We'll need to probably add ${"-webkit-foo -moz-foo" if gecko else ""} all over the place. Might want to come up with a convenient attibute name for that (gecko_prefix="mw") or something.

r? @heycam or @bholley


This change is Reviewable

@highfive
Copy link

highfive commented Jan 6, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/shorthand/inherited_text.mako.rs, components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/build.py, components/style/properties/properties.mako.rs, components/style/properties/data.py
  • @emilio: components/style/properties/shorthand/inherited_text.mako.rs, components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/build.py, components/style/properties/properties.mako.rs, components/style/properties/data.py

@highfive
Copy link

highfive commented Jan 6, 2017

warning Warning warning

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

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 6, 2017
@emilio
Copy link
Member

emilio commented Jan 6, 2017

I'm concerned about serialization, should we serialize as the alias?

@Manishearth
Copy link
Member Author

This was discussed yesterday, and the conclusion was that aliases should disappear when serializing.

In firefox:

    #foo {
        word-wrap: break-word;
        overflow-wrap: break-word;
        align-items: flex-end;
        -webkit-align-items: baseline;
        foobar: baz;
    }

serializes to "#foo { overflow-wrap: break-word; align-items: baseline; }" (document.styleSheets[0].cssRules[0].cssText)

@Manishearth
Copy link
Member Author

Specifically for word-wrap this behavior is actually wrong. Spec says:

For legacy reasons, UAs must treat word-wrap as an alternate name for the overflow-wrap property, as if it were a shorthand of overflow-wrap.

However, Firefox implements word-wrap as an alias of overflow-wrap. Chrome and Safari implement it as a shorthand.

Prefix aliases are not specced but behave consistently across browsers (the irony). The alias mapping is done at parse time, and no trace of their existence is left behind. CSSOM allows access to the same property through regular and alias accessors. This is what this PR does.

So perhaps we shouldn't be using this alias infra for word-wrap. The infra must exist for prefix aliases to work, but we don't need to use it for word-wrap.

We should probably update Gecko to use a shorthand here. And change this PR to use -webkit-align-items or something?

@Manishearth
Copy link
Member Author

Gecko bug that made it an alias is in https://bugzilla.mozilla.org/show_bug.cgi?id=955857 . It doesn't seem like making it not a shorthand was a deliberate decision, it was a different interpretation with the spec.

Filed w3c/csswg-drafts#866 to deal with the spec ambiguities. Not sure how that will turn out, but we would prefer it to be specced so that it is an alias, not a shorthand.

@Manishearth
Copy link
Member Author

Turns out that basically nobody follows the "implement it as a shorthand" thing correctly anyway. Chrome and webkit do their own special-snowflake thing that only is used for word-wrap. I'm going to leave it as is as an alias since that's the direction the spec discussion is going in.

@emilio
Copy link
Member

emilio commented Jan 7, 2017

Thanks Manish, makes sense.

So the idea was that StaticId would be merged into LonghandId eventually. With this patch we can't, but I think I'm personally fine with this?

I guess you should ask for the sign-off of @SimonSapin (or not? IDK), and at least remove that FIXME comment that says we'll use LonghandId.

@emilio
Copy link
Member

emilio commented Jan 7, 2017

Or, better than removing it, document the news setup, i.e., StaticId contains alias, but LonghandId not.

@emilio
Copy link
Member

emilio commented Jan 7, 2017

Disregard those last two comments, too sleepy to see that static_id_generator actually didn't generate new StaticIds, but pointed to the appropriate longhand's.

Sorry :(

@emilio
Copy link
Member

emilio commented Jan 7, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 78cb160 has been approved by emilio

@highfive highfive assigned emilio and unassigned heycam Jan 7, 2017
@highfive 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 Jan 7, 2017
@emilio
Copy link
Member

emilio commented Jan 7, 2017

@bors-servo r-

./components/style/properties/build.py:81: E302 expected 2 blank lines, found 1

Fails tidy, not reason to take space in the queue, feel free to r=me when fixed.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 7, 2017
@Manishearth
Copy link
Member Author

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

📌 Commit 3081bc9 has been approved by emilio

@highfive 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 Jan 7, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 3081bc9 with merge 0fc0224...

bors-servo pushed a commit that referenced this pull request Jan 7, 2017
Support property aliases

Aliases now get forwarded to their "original" property name at parse time. CSSOM continues to provide access through alias getters.

We'll need to probably add `${"-webkit-foo -moz-foo" if gecko else ""}` all over the place. Might want to come up with a convenient attibute name for that (`gecko_prefix="mw"`) or something.

r? @heycam or @bholley

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14893)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-dev-unit

@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 7, 2017
@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 7, 2017
@highfive 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 Jan 7, 2017
@Manishearth
Copy link
Member Author

@bors-servo r=emilio

removed a test that doesn't make sense anymore

@bors-servo
Copy link
Contributor

📌 Commit 33966a8 has been approved by emilio

@highfive 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 Jan 7, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 33966a8 with merge d14158a...

bors-servo pushed a commit that referenced this pull request Jan 7, 2017
Support property aliases

Aliases now get forwarded to their "original" property name at parse time. CSSOM continues to provide access through alias getters.

We'll need to probably add `${"-webkit-foo -moz-foo" if gecko else ""}` all over the place. Might want to come up with a convenient attibute name for that (`gecko_prefix="mw"`) or something.

r? @heycam or @bholley

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14893)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

@bors-servo bors-servo merged commit 33966a8 into servo:master Jan 7, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 7, 2017
@SimonSapin
Copy link
Member

So the idea was that StaticId would be merged into LonghandId eventually.

From properties.mako.rs:

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

// ...

pub enum PropertyId {
    Longhand(LonghandId),
    Shorthand(ShorthandId),
    Custom(::custom_properties::Name),
}

(custom_properties::Name has a destructor, for atom reference counting.)

@Manishearth Manishearth deleted the alias branch January 9, 2017 14:14
@Manishearth
Copy link
Member Author

Yeah, that's compatible with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants