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 appearance: textfield #14994

Merged
merged 1 commit into from Aug 13, 2019
Merged

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Jan 22, 2019

@jugglinmike
Copy link
Contributor

If I understand your use of appearance: none correctly, it seems like a neat pattern. It took me a few minutes to work out, though, which makes me think it might be susceptible to removal by future maintainers. Is it a common pattern in CSS testing?

If not, a comment would go a long way. Something like, "For most elements, the expected behavior identical to the initial value. Initially set the property value to none to verify that this is value is being interpreted (instead of simply ignored)."

@zcorpan
Copy link
Member Author

zcorpan commented Jun 27, 2019

I don't know if it's common in the css testsuite to rely on this particular behavior, but regardless, a comment seems like a good idea.

I think it could be stated more simply

/* If the value being tested is not supported, then 'none' is used instead, which is intended to fail the test. */

Also I think this test needs

  • rebasing
  • change -webkit-appearance to appearance
  • run the build script to generate the -webkit-appearance variant of this test

@zcorpan zcorpan assigned jugglinmike and unassigned svgeesus Jun 27, 2019
@svgeesus svgeesus removed their request for review June 28, 2019 22:54
@jugglinmike jugglinmike force-pushed the zcorpan/appearance-textfield branch from 21529b5 to cce2afd Compare July 3, 2019 20:52
@jugglinmike
Copy link
Contributor

I followed up with @zcorpan's suggestions. That makes me a co-author, so I'd like further review before merging. @frivoal @mrego @plinss @tantek would one of you mind giving this a look?

(The prior version of this branch is available here.)

@zcorpan
Copy link
Member Author

zcorpan commented Aug 13, 2019

The change LGTM, @jugglinmike . I can't mark as reviewed, but I can admin-merge.

@zcorpan zcorpan merged commit 9f01716 into master Aug 13, 2019
@jugglinmike
Copy link
Contributor

Thanks!

@gsnedders gsnedders deleted the zcorpan/appearance-textfield branch September 10, 2019 17:23
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.

None yet

4 participants