Support :disabled and :enabled pseudo-classes

RESOLVED FIXED in Future

Status

()

Core
CSS Parsing and Computation
P3
enhancement
RESOLVED FIXED
16 years ago
7 years ago

People

(Reporter: Skewer, Assigned: Allan Beaufour)

Tracking

(Depends on: 1 bug, {css3, fixed1.8, testcase})

Trunk
Future
css3, fixed1.8, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-PF])

Attachments

(6 attachments, 6 obsolete attachments)

1.66 KB, text/html
Details
24.25 KB, patch
Details | Diff | Splinter Review
24.20 KB, patch
Mike Schroepfer
: approval1.8b4-
Mike Schroepfer
: approval1.8b5+
Details | Diff | Splinter Review
6.59 KB, application/xhtml+xml
Details
2.54 KB, application/xhtml+xml
Details
18.84 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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
(Reporter)

Comment 1

16 years ago
Created attachment 37451 [details]
CSS3 Disabled pseudo-class testcase
(Reporter)

Comment 2

16 years ago
Keywords: testcase, css3
Keywords: css3, testcase
Blocks: 65133
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.

Updated

13 years ago
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?
CSSWG contacted.
http://lists.w3.org/Archives/Member/w3c-css-wg/2004OctDec/0198.html

Updated

13 years ago
Blocks: 271720

Comment 10

12 years ago
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.)
(Assignee)

Comment 12

12 years ago
Created attachment 190540 [details] [diff] [review]
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)
Assignee: dbaron → allan
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #190540 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

12 years ago
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)
(Assignee)

Comment 14

12 years ago
Created attachment 190549 [details] [diff] [review]
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"
Attachment #190540 - Attachment is obsolete: true
Attachment #190549 - Flags: review?(bzbarsky)

Comment 15

12 years ago
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.
(Assignee)

Comment 16

12 years ago
Created attachment 190821 [details]
Testcase
(Assignee)

Comment 17

12 years ago
(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

12 years ago
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.
Same question here as in bug 302188 comment 21.
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...
Created attachment 194143 [details] [diff] [review]
Updated to tip
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.
Created attachment 194145 [details] [diff] [review]
Updated to all my comments
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"...
(Assignee)

Comment 27

12 years ago
(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?
(Assignee)

Comment 29

12 years ago
(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 31

12 years ago
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-
(Assignee)

Updated

12 years ago
Blocks: 306620
(Assignee)

Comment 32

12 years ago
(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
(Assignee)

Comment 33

12 years ago
(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

12 years ago
(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.
(Assignee)

Updated

12 years ago
Attachment #194145 - Flags: approval1.8b5?

Comment 37

12 years ago
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?
(Assignee)

Comment 38

12 years ago
Created attachment 197389 [details]
Testcase: Controls matching enabled/disabled
Attachment #190821 - Attachment is obsolete: true
(Assignee)

Comment 39

12 years ago
Created attachment 197390 [details]
Testcase: Controls _not_ matching enabled/disabled
(Assignee)

Updated

12 years ago
Flags: testcase?
(Assignee)

Updated

12 years ago
Attachment #194145 - Flags: approval1.8b5?

Comment 40

12 years ago
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+

Updated

12 years ago
Flags: blocking1.8b5+
(Assignee)

Comment 41

12 years ago
Oops, uploaded old versions of testcases that do not incorporate bug 306620. New
ones one the way.
(Assignee)

Comment 42

12 years ago
Created attachment 197532 [details]
Testcase: Controls matching enabled/disabled v2
Attachment #197389 - Attachment is obsolete: true
(Assignee)

Comment 43

12 years ago
Created attachment 197533 [details]
Testcase: Controls _not_ matching enabled/disabled v2
Attachment #197390 - Attachment is obsolete: true
(Assignee)

Comment 44

12 years ago
checked in to 1.8 branch
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Updated

12 years ago
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.
(Assignee)

Comment 46

11 years ago
(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

10 years ago
Created attachment 260441 [details] [diff] [review]
Simple reftest v1
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-
Created attachment 261322 [details] [diff] [review]
Reftests based on attachment 197532 [details] and attachment 197533 [details]
Attachment #260441 - Attachment is obsolete: true
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.