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

Test that :read-only matches <input>s to which [readonly] doesn't apply #2843

Merged
merged 1 commit into from
Apr 19, 2016

Conversation

cvrebert
Copy link
Contributor

This tests the other side of #2839.
As a reminder, per https://html.spec.whatwg.org/multipage/scripting.html#selector-read-only

The :read-write pseudo-class must match any element falling into one of the following categories [...]
The :read-only pseudo-class must match all other HTML elements.

Refs https://bugs.chromium.org/p/chromium/issues/detail?id=604154
Refs https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/7229941/

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/6400

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@wpt-pr-bot
Copy link
Collaborator

Reviewers for this pull request are: @Ms2ger, @foolip, @gsnedders, @jdm, @jgraham, @plehegar, @sideshowbarker, @zcorpan, and @zqzhang.

@zcorpan
Copy link
Member

zcorpan commented Apr 18, 2016

Thanks for this! The tests match the spec AFAICT, but it's not clear to me that implementors want to match the spec. It's also pretty useless to have a selector for :read-only if it just means :not(:read-write). But maybe it makes sense somehow? cc @Hixie @tabatkins

@cvrebert
Copy link
Contributor Author

Yes, I agree that the spec isn't particularly reasonable here, but didn't dig in to evaluate whether the weirdness is justified or not (perhaps there's a history here).
Note that Safari currently implements this as-currently-spec'd, unprefixed.

it's not clear to me that implementors want to match the spec.

Given their response to https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/7229941/ , the Edge folks currently seem to be in the "the spec is bad" camp.

@zcorpan
Copy link
Member

zcorpan commented Apr 18, 2016

This changed in WebKit in WebKit/WebKit@c097867

@BenjaminPoulain what are your thoughts? Also see @FremyCompany's comment in the Edge issue

Maybe we should take this to www-style?

@BenjaminPoulain
Copy link

It's also pretty useless to have a selector for :read-only if it just means :not(:read-write).

That's pretty common. Take ":link" and ":visited" for example.

I would expect :read-only and :read-write to be opposite, as it is for any permission system. That what follows the principle of least astonishment IMHO.

Maybe we should take this to www-style?

Yep. I do not intend to change WebKit to go against the spec.

@zcorpan
Copy link
Member

zcorpan commented Apr 19, 2016

:link and :visited are mutually exclusive but are not the negation of each other; neither of them mathes a div, for instance. They only match hyperlinks. Similarly it seems reasonable to assume that :read-only would only math elements that are potentially writeable but are currently in a readonly state.

With contenteditable I suppose it's possible to argue that any element can become writeable, though.

@zcorpan
Copy link
Member

zcorpan commented Apr 19, 2016

I'll merge this since it's correct per the current spec. I can start a new thread in www-style.

@zcorpan zcorpan merged commit bbcf969 into master Apr 19, 2016
@zcorpan zcorpan deleted the read-only-pseudo branch April 19, 2016 07:26
@cvrebert
Copy link
Contributor Author

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.

5 participants