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

[css-ui] Test serialization: -webkit-appearance #17221

Merged

Conversation

jugglinmike
Copy link
Contributor

This is in service of csswg-drafts #3024 - Unprefix 'appearance' and/or make the spec web-compatible

I've tried to avoid the parsing code paths from the serialization tests and vice versa. Was I successful?

Copy link
Member

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also test the same things but for the unprefixed 'appearance' property?

<script>
test(function() {
var input = document.createElement('input');
input.style = '-webkit-appearance: none;';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to use setAttribute() instead. The setter here is forwarded to the cssText setter.

@zcorpan
Copy link
Member

zcorpan commented Jun 9, 2019

I've tried to avoid the parsing code paths from the serialization tests and vice versa. Was I successful?

I believe so, but I don't really know how these things are implemented to say for sure. :-)

@zcorpan zcorpan assigned zcorpan and unassigned plinss Jun 9, 2019
@jugglinmike
Copy link
Contributor Author

This needs to use setAttribute() instead. The setter here is forwarded to the cssText setter.

The path I'm trying to test is the attribute change steps for the owner node:

https://drafts.csswg.org/cssom/#ref-for-parse-a-css-declaration-block

By my reading, the relationship is the other way around--cssText seems to depend on the change steps:

The cssText attribute must return the result of serializing the declarations.
Setting the cssText attribute must run these steps:

[...]

  1. Update style attribute for the CSS declaration block.

[...]

To update style attribute for declaration block means to run the steps below:

[...]

  1. Set an attribute value for owner node using "style" and the result of
    serializing declaration block.

That's why using cssText seems like a less direct way of testing the same thing.

Can you also test the same things but for the unprefixed 'appearance' property?

As I understand it, such tests would verify that appearance is parsed as appearance and that appearance is serialized as appearance. Are there more nuanced expectations than that? If so, could you point me to where they are defined or proposed? If not, could you say a bit about what would make that an interesting test case for parsing or serialization?

@zcorpan
Copy link
Member

zcorpan commented Jun 10, 2019

The attribute change steps are run as a result of setAttribute() per the DOM spec.

https://dom.spec.whatwg.org/#dom-element-setattribute
https://dom.spec.whatwg.org/#concept-element-attributes-change
https://dom.spec.whatwg.org/#concept-element-attributes-change-ext

Setting elm.style is exactly the same as setting elm.style.cssText, since the former is annotated with [PutForwards=cssText] WebIDL extended attribute.

https://drafts.csswg.org/cssom/#elementcssinlinestyle

As for appearance, yes it should parse and serialize as appearance, but also calling getPropertyValue('-webkit-appearance') should also work. Basically I would test the exact same things for both appearance and -webkit-appearance inputs, to verify that they're properly aliased.

Change API used to set style in order to actually test the internals
referenced by the test title.
Introduce tests for property reference
Comprehensive tests for property references have been added since these
tests were written. Remove the excess assertions in order to improve the
focus of these tests.
Add equivalent tests for unprefixed `appearance` property
@jugglinmike
Copy link
Contributor Author

Thanks for the clarification, Simon! I've improved the tests using a few commits and added an explanation to the body of each commit message.

This introduces new tests for direct property reference. I've added these in a new file because according to legacy name alias, they are neither parsing nor serialization. This motivated a lot of tests (just learned that "webkit-case" is a concept in CSSOM), and a dedicated file helps make the enumeration of all combinations (including how the methods should not recognize the camel-case/webkit-case versions) a little easier to see.

Since the serialization and parsing tests already use these API's, I didn't rely on serialization or parsing here. That lead to some overlap (i.e. multiple sub-tests rely on setProperty--not just the sub-test dedicated to it), but I think this is unavoidable.

These new tests also made some of the assertions in the "parsing" tests redundant. I've removed those extra assertions to keep things organized.

What do you think?

Copy link
Member

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seal of approval

@jugglinmike
Copy link
Contributor Author

I will cherish this all the days of my life

@zcorpan
Copy link
Member

zcorpan commented Jun 19, 2019

I think we can merge this.

@zcorpan zcorpan merged commit 79787d6 into web-platform-tests:master Jun 19, 2019
marcoscaceres pushed a commit that referenced this pull request Jul 23, 2019
* [css-ui] Test serialization: `-webkit-appearance`

* fixup! [css-ui] Test serialization: `-webkit-appearance`

Change API used to set style in order to actually test the internals
referenced by the test title.

* fixup! [css-ui] Test serialization: `-webkit-appearance`

Introduce tests for property reference

* fixup! [css-ui] Test serialization: `-webkit-appearance`

Remove duplicate assertion

* fixup! [css-ui] Test serialization: `-webkit-appearance`

Comprehensive tests for property references have been added since these
tests were written. Remove the excess assertions in order to improve the
focus of these tests.

* fixup! [css-ui] Test serialization: `-webkit-appearance`

Add equivalent tests for unprefixed `appearance` property

* fixup! [css-ui] Test serialization: `-webkit-appearance`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants