888884 - :-moz-read-write pseudo-class shouldn't be applied on <input type=text disabled> and <textarea disabled>

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Makoto Kato [:m_kato]]

WHATWG spec says,

The :read-write pseudo-class must match any element falling into one of the following categories, which for the purposes of Selectors are thus considered user-alterable: [SELECTORS]

    input elements to which the readonly attribute applies, and that are mutable (i.e. that do not have the readonly attribute specified and that are not disabled)


But :-moz-read-write is applyed on <input type=text disabled>.

Same issue occurs on Blink and WebKit. (http://code.google.com/p/chromium/issues/detail?id=255351)

Comment 1

[Boris Zbarsky [:bzbarsky]]

The spec here may well be bogus; I recall there being issues raised on it in the past.

In any case, this is a pure form controls bug: the element claims to be in that state, so the selector matches it.  There's nothing related to CSS proper going on here.  See the IntrinsicState methods.

Comment 2

[Mounir Lamouri (:mounir)]

Actually, we are thinking of simply removing the :read-write and :read-only selectors from HTML. Last I've heard, Hixie is was asking the CSS editors if they would agree to remove the selectors from CSS specifications.

If that happen, the specifications would not matter much.

See: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17812

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Part 1. - Apply :-moz-read-only on disabled edtiable text

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Part 2. Add reftest

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8693464 [details] [diff] [review]
Part 1. - Apply :-moz-read-only on disabled edtiable text

Don't set NS_EVENT_STATE_MOZ_READWRITE that editable element has disabled attirubte.

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 8693465 [details] [diff] [review]
Part 2. Add reftest

Add reftest for input type="text" and textarea

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8693464 [details] [diff] [review]
Part 1. - Apply :-moz-read-only on disabled edtiable text

What was the outcome of the spec discussion?

What sort of checking have you done to make sure this won't break content, both on the web and addons?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8693465 [details] [diff] [review]
Part 2. Add reftest

r=me assuming we make this change.

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8693464 [details] [diff] [review]
> Part 1. - Apply :-moz-read-only on disabled edtiable text
> 
> What was the outcome of the spec discussion?
> 
> What sort of checking have you done to make sure this won't break content,
> both on the web and addons?

- Blink wants to keep same behavior as Firefox and old Opera.  It is for web compatibility.　(https://code.google.com/p/chromium/issues/detail?id=255351)
- WebKit already changed to HTML5 spec's behavior (disabled applys :read-only) at 2 year ago.  But I cannot find break the web by this change. 
- HTML5 spec unfortunately says as same as WHATWG spec (http://www.w3.org/TR/html5/disabled-elements.html).

Also ,we still use prefix for this. But Blink and WebKit doesn't use prefix and both is different behavior now.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8693464 [details] [diff] [review]
Part 1. - Apply :-moz-read-only on disabled edtiable text

> - Blink wants to keep same behavior as Firefox and old Opera.

Or at least one Blink developer does.  What about IE?

I would really like us to change this no more than once.  Please talk to the Blink developers about whether they're willing to get on board and make this change if we do, so we don't end up having to revert this change later.

Please re-request review once we have a commitment from Blink, ok?

Comment 11

[Makoto Kato [:m_kato]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8693464 [details] [diff] [review]
> Part 1. - Apply :-moz-read-only on disabled edtiable text
> 
> > - Blink wants to keep same behavior as Firefox and old Opera.
> 
> Or at least one Blink developer does.  What about IE?

Edge 13 is first implementation for it and is same as Blink.

> I would really like us to change this no more than once.  Please talk to the
> Blink developers about whether they're willing to get on board and make this
> change if we do, so we don't end up having to revert this change later.

OK.  I should discuss it with google.  Maybe, we should change the spec (HTML5.1, CSS selector 4 and WHATWG) after discussion?

> Please re-request review once we have a commitment from Blink, ok?

OK.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Maybe, we should change the spec (HTML5.1, CSS selector 4 and WHATWG) after discussion?

If no one except Safari is willing to implement it, then yes...

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 888884 - Make :read-write not apply for disabled controls to which readonly applies. r=smaug

Per https://html.spec.whatwg.org/#selector-read-write:

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

• input elements to which the readonly attribute applies, and that
  are mutable (i.e. that do not have the readonly attribute
  specified and that are not disabled)

• textarea elements that do not have a readonly attribute, and
  that are not disabled.

• elements that are editing hosts or editable and are neither
  input elements nor textarea elements.

This fixes the :disabled bits.

This matches Safari behavior and the spec, but not Chrome, which has our
behavior.

Fix the WPT, which had multiple issues, as :read-only is defined to be
just the opposite of :read-write. This will pass as soon as I unprefix
the pseudo-class.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b891abd233e6
Make :read-write not apply for disabled controls to which readonly applies. r=edgar

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/23603 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1eb80ef4079
Mark the newly added WPTs as failing for now.

Comment 18

[David Lawrence [:dkl]]

Attachment: Bug 888884 - Mark the newly added WPTs as failing for now.

As the pseudo-class is still not un-prefixed.

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b891abd233e6
https://hg.mozilla.org/mozilla-central/rev/e1eb80ef4079

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 21

[Kohei Yoshino]

Mentioned this in the site compatibility note for Bug 312971.

Comment 22

[Chris Mills [:cmills]]

I've added a rel note to cover this: https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/78#CSS

I don't think anything else is needed here.