Closed Bug 84400 Opened 19 years ago Closed 14 years ago

Support :disabled and :enabled pseudo-classes

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: SkewerMZ, Assigned: allan)

References

(Depends on 1 open bug)

Details

(Keywords: css3, fixed1.8, testcase, Whiteboard: [Hixie-PF])

Attachments

(6 files, 6 obsolete files)

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
Keywords: testcase, css3
Keywords: css3, testcase
Blocks: selectors3
Severity: minor → enhancement
OS: Windows 98 → All
Hardware: PC → All
Summary: [CSS3] Support :disabled and :enabled pseudo-classes → Support :disabled and :enabled pseudo-classes [SELECTORS]
Whiteboard: [Hixie-PF]
Target Milestone: --- → Future
Depends on: 115462
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
Priority: -- → P3
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....
Keywords: qawanted
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.
Summary: Support :disabled and :enabled pseudo-classes [SELECTORS] → Support :disabled and :enabled pseudo-classes
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...
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).
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?
Blocks: 271720
Hixie: has the WG come to a conclusion about this?
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.)
Attached patch Patch (obsolete) — Splinter Review
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)
Assignee: dbaron → allan
Status: NEW → ASSIGNED
Attachment #190540 - Flags: review?(bzbarsky)
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.
Attachment #190540 - Flags: review?(bzbarsky)
Attached patch Patch v2 (obsolete) — Splinter Review
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"
Attachment #190540 - Attachment is obsolete: true
Attachment #190549 - Flags: review?(bzbarsky)
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.
Attached file Testcase (obsolete) —
(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
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 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.
Attachment #190549 - Flags: superreview+
Attachment #190549 - Flags: review?(bzbarsky)
Attachment #190549 - Flags: review+
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...
Attached patch Updated to tipSplinter Review
Attachment #190549 - Attachment is obsolete: true
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.
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.
"kinda like to see" means "will need to write before I land this unless someone
else gets there first"...
(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.
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?
(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 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...
Attachment #194145 - Flags: approval1.8b4?
Comment on attachment 194145 [details] [diff] [review]
Updated to all my comments

Locking down for beta - not taking any new features at this point.
Attachment #194145 - Flags: approval1.8b4? → approval1.8b4-
Blocks: 306620
(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).
No longer blocks: 306620
Depends on: 306614, 306620
(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.
(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.)
Hmm.. This caused bug 307249
Depends on: 307249
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.
Attachment #194145 - Flags: approval1.8b5?
Comment on attachment 194145 [details] [diff] [review]
Updated to all my comments

please rerequest approval when this is in better shape. Thanks.
Attachment #194145 - Flags: approval1.8b5?
Attachment #190821 - Attachment is obsolete: true
Flags: testcase?
Attachment #194145 - Flags: approval1.8b5?
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.
Attachment #194145 - Flags: approval1.8b5? → approval1.8b5+
Flags: blocking1.8b5+
Oops, uploaded old versions of testcases that do not incorporate bug 306620. New
ones one the way.
Attachment #197389 - Attachment is obsolete: true
Attachment #197390 - Attachment is obsolete: true
checked in to 1.8 branch
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Keywords: qawanted
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.
(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.
Attached patch Simple reftest v1 (obsolete) — Splinter Review
Attachment #260441 - Flags: review?(bzbarsky)
I probably won't get to this until a week or two from now.
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....
Attachment #260441 - Flags: review?(bzbarsky) → review-
Checked in those tests.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.