84400 - Support :disabled and :enabled pseudo-classes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Skewer]

Procedure: View the testcase.

Expected: Enabled textbox will be green, disabled textbox will be red.

Actual: :disabled and :enabled pseudo-classes are not implemented.

Build: 2001060604 Win98

Comment 1

[Skewer]

Attachment: CSS3 Disabled pseudo-class testcase

Comment 2

[Skewer]

Keywords: testcase, css3

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Assigning pierre's remaining Style System-related bugs to myself.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So... the hard part here is deciding what these selectors should apply to, no?

So:
 links (note that there is no way to disable these, really)
 form controls

what else?

Also, what's the interaction with -moz-user-focus here?  The text at
http://www.w3.org/TR/2001/CR-css3-selectors-20011113/#UIstates makes me think
that whether these selectors apply depends on whether the element can be
focused, but -mos-user-focus affects that....

Comment 5

[Hixie (not reading bugmail)]

We should implement the proposal in bug 115462 and base this bug's fix on that.
IMHO. But then I would say that, since said proposal is mine...

Hyatt and I have since proposed that for CSS3.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So... All HTML nodes can be focusable if a nonnegative tabindex is set.  Per
CSS3, that means they should all match :disabled (or :enabled if tabindex is
indeed set or they're enabled form controls or anchors, etc), right?  See the
"An element is enabled if the user can either activate it or transfer the focus
to it. An element is disabled if it could be enabled, but the user cannot
presently activate it or transfer focus to it" language in the spec...

Comment 7

[Hixie (not reading bugmail)]

tabindex doesn't set if it's focussable, it sets if it is tabbable. (Which can
also imply it's focussable when it might normally not be, but that's another
story.) It seems kinda weird for :disabled to apply to everything, though. Maybe
we should "clarify" this clause in the spec (or alternatively clarify "tabindex"
in a whatwg spec, defining its implications on :disabled).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yes, some clarification would be nice.  I was all set to implement this, and
then discovered this issue...  I agree that having the world match :disabled is
silly, so some clarity one way or the other would be appreciated.  On the other
hand, what do we mean by "focusable"?  How does this interact with caret
browsing, say?

Comment 9

[Hixie (not reading bugmail)]

CSSWG contacted.
http://lists.w3.org/Archives/Member/w3c-css-wg/2004OctDec/0198.html

Comment 10

[Anne (:annevk)]

Hixie: has the WG come to a conclusion about this?

Comment 11

[Hixie (not reading bugmail)]

Not really. I couldn't really convince the CSSWG members that there was an
issue. It was suggested that allowing tabindex="" on everything was wrong.

I guess we'll have to define this in the WHATWG spec instead. I'd say things
should be defined something as follows: "An element is enabled if the user can
either activate it or transfer the focus to it. An element is disabled if its
enabled state has been explicitly overriden."

If you can find better wording I'm all for it. I hope to just list the relevant
attributes in the WHATWG spec. ("This makes it enabled, this makes it disabled,
this makes it nothing at all, ..." etc.)

Comment 12

[Allan Beaufour]

Attachment: Patch

I just check for the presence of the disabled attribute, like stated in:
http://whatwg.org/specs/web-forms/current-work/#relation

Notes:
* does not check ancestors

* matches _all_ elements inheriting from nsGenericHTMLFormElement
  Per the WebForms 2 spec. the only exception is output, which we do not have
(yet)

Comment 13

[Allan Beaufour]

Comment on attachment 190540 [details] [diff] [review]
Patch

(In reply to comment #12)
> Created an attachment (id=190540) [edit]
>
> * matches _all_ elements inheriting from nsGenericHTMLFormElement
>   Per the WebForms 2 spec. the only exception is output, which we do not have
> (yet)

Ach. From Web Forms 2: "The term form control refers to input, output, select,
textarea and button", so I'll need to check that.

Comment 14

[Allan Beaufour]

Attachment: Patch v2

Ok this adds nsGenericHTMLFormElement::IsFormControlOrFieldSet(),
used by AfterSetAttr() and GetIntrinsicState() (forcing GetType() to be const,
which makes sense anyway).

So the only thing that this patch does not handle is checking for "disabled by
inheritance"

Comment 15

[Anne (:annevk)]

Some testcases:
 <http://annevankesteren.nl/test/css/ui/006>
 <http://annevankesteren.nl/test/css/ui/007>

As we do not do inheritence yet the latter should probably show no green at all.

Comment 16

[Allan Beaufour]

Attachment: Testcase

Comment 17

[Allan Beaufour]

(In reply to comment #14)
> Created an attachment (id=190549) [edit]
> Patch v2
> 
> Ok this adds nsGenericHTMLFormElement::IsFormControlOrFieldSet(),
> used by AfterSetAttr() and GetIntrinsicState() (forcing GetType() to be const,
> which makes sense anyway).
> 
> So the only thing that this patch does not handle is checking for "disabled by
> inheritance"

Which btw is WF2 only, as the disabled attribute is not defined for fieldsets in
HTML4

Comment 18

[Anne (:annevk)]

Correct. Form controls inside a disabled fieldset element are not implicitly
disabled in Mozilla. Perhaps this is something for 1.9 if we are doing WF2 by then.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Same question here as in bug 302188 comment 21.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 190549 [details] [diff] [review]
Patch v2

>Index: content/html/content/src/nsGenericHTMLElement.h

>+  /**
>+   * Called when an attribute has just been changed.
>+   *
>+   * Note that the function is also called if the attribute change fails.

s/the function/this function/

r+sr=bzbarsky with that.  I'll try to check this in in the next day or two.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 190549 [details] [diff] [review]
Patch v2

Though this would have been much easier to review if there were a diff -w
version attached as well...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194143 [details] [diff] [review]
Updated to tip

>Index: content/events/public/nsIEventStateManager.h
>+nsGenericHTMLFormElement::IsFormControlOrFieldSet() const
>+  if (type == NS_FORM_LABEL ||
>+      type == NS_FORM_OPTION ||

This could just return the right boolean, I just realized... I'll make that
change.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to all my comments

Comment 25

[Boris Zbarsky [:bzbarsky]]

Anne, may I check your testcases into the layout regression tests?

Other tests I'd kinda like to see here:

1)  Tests ensuring that random elements match neither :disabled nor :enabled
2)  Tests ensuring that all the form controls that should match do (not just
    <input>).
3)  Tests ensuring that form controls that don't match do not.

The full list of form control types is found at
http://lxr.mozilla.org/seamonkey/source/content/html/content/public/nsIFormControl.h#49
if that will help.

Note that none of the tests should require user interaction to run, since I want
to add them to the automated regression tests.

Comment 26

[Boris Zbarsky [:bzbarsky]]

"kinda like to see" means "will need to write before I land this unless someone
else gets there first"...

Comment 27

[Allan Beaufour]

(In reply to comment #21)
> (From update of attachment 190549 [details] [diff] [review] [edit])
> Though this would have been much easier to review if there were a diff -w
> version attached as well...

:( I actually promised you that on one of the other bugs, didn't I? I forgot all
about it, sorry for that.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I checked in the patch.  Still waiting on Anne to find out whether I can check
in the tests he wrote as regression tests.

Allan, I assume you want to try for 1.8 approval for this?

Comment 29

[Allan Beaufour]

(In reply to comment #28)
> I checked in the patch.

Thanks!

> Allan, I assume you want to try for 1.8 approval for this?

Yes.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194145 [details] [diff] [review]
Updated to all my comments

Requesting 1.8 approval.  This should be pretty safe, and very useful for
XForms...

Comment 31

[Mike Schroepfer]

Comment on attachment 194145 [details] [diff] [review]
Updated to all my comments

Locking down for beta - not taking any new features at this point.

Comment 32

[Allan Beaufour]

(In reply to comment #25)
> Anne, may I check your testcases into the layout regression tests?
> 
> Other tests I'd kinda like to see here:
> 
> 1)  Tests ensuring that random elements match neither :disabled nor :enabled
> 2)  Tests ensuring that all the form controls that should match do (not just
>     <input>).
> 3)  Tests ensuring that form controls that don't match do not.

I've created some tests here:
http://beaufour.dk/tmp/
I need to check option, optgroup, and legend, but the rest is there I think.

The "matching enabled/disabled" one exibits weird behaviour on reload, but
that's because of form restoration. I'm not sure it's entirely correct behaviour
(see bug 306616).

Comment 33

[Allan Beaufour]

(In reply to comment #32)
> I've created some tests here:
> http://beaufour.dk/tmp/
> I need to check option, optgroup, and legend, but the rest is there I think.

Included the rest now.

Comment 34

[Anne (:annevk)]

(In reply to comment #25)
> Anne, may I check your testcases into the layout regression tests?

Yes. (I will try to make more testcases later using your guidelines. Might take
some weeks though.)

Comment 35

[Boris Zbarsky [:bzbarsky]]

Hmm.. This caused bug 307249

Comment 36

[Boris Zbarsky [:bzbarsky]]

Note that the regressions and followups (the bugs blocking this one with 3xxxxx
numbers) need to be addressed before we have a good idea of how safe this patch
is for branch.

Comment 37

[Asa Dotzler [:asa]]

Comment on attachment 194145 [details] [diff] [review]
Updated to all my comments

please rerequest approval when this is in better shape. Thanks.

Comment 38

[Allan Beaufour]

Attachment: Testcase: Controls matching enabled/disabled

Comment 39

[Allan Beaufour]

Attachment: Testcase: Controls _not_ matching enabled/disabled

Comment 40

[Mike Schroepfer]

Comment on attachment 194145 [details] [diff] [review]
Updated to all my comments

Approved per 9/26 bug triage.  Please make sure to land the regression fixes at
the same time.

Comment 41

[Allan Beaufour]

Oops, uploaded old versions of testcases that do not incorporate bug 306620. New
ones one the way.

Comment 42

[Allan Beaufour]

Attachment: Testcase: Controls matching enabled/disabled v2

Comment 43

[Allan Beaufour]

Attachment: Testcase: Controls _not_ matching enabled/disabled v2

Comment 44

[Allan Beaufour]

checked in to 1.8 branch

Comment 45

[Boris Zbarsky [:bzbarsky]]

Did the testcases ever get added to the regression tests?  If not, they should be.  Please do that.  That includes the testcases Anne has posted links to in the bug, since he's said he's ok with adding them to the regression tests.

Comment 46

[Allan Beaufour]

(In reply to comment #45)
> Did the testcases ever get added to the regression tests?  If not, they should
> be.  Please do that.  That includes the testcases Anne has posted links to in
> the bug, since he's said he's ok with adding them to the regression tests.

I've just checked them in.

Comment 47

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Simple reftest v1

Comment 48

[Boris Zbarsky [:bzbarsky]]

I probably won't get to this until a week or two from now.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 260441 [details] [diff] [review]
Simple reftest v1

That doesn't so much test anything, since the color doesn't apply to anything in those testcases....

Comment 50

[Boris Zbarsky [:bzbarsky]]

Attachment: Reftests based on attachment 197532 and attachment 197533

Comment 51

[Boris Zbarsky [:bzbarsky]]

Checked in those tests.