Last Comment Bug 759666 - HTML option doesn't have disabled state inherited from disabled optgroup
: HTML option doesn't have disabled state inherited from disabled optgroup
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: statea11y 577306
  Show dependency treegraph
 
Reported: 2012-05-29 23:28 PDT by alexander :surkov
Modified: 2012-06-02 11:51 PDT (History)
3 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (12.91 KB, patch)
2012-05-31 11:50 PDT, Mounir Lamouri (:mounir)
bzbarsky: review+
mounir: checkin+
Details | Diff | Splinter Review

Description alexander :surkov 2012-05-29 23:28:35 PDT
HTMLOptionElement->State().HasState(NS_EVENT_STATE_DISABLED) is false:

<select size="3" disabled>
  <option>option</option>
</select>

<select size="3">
  <optgroup disabled>
    <option>option</option>
  </optgroup>
</select>
Comment 1 Boris Zbarsky [:bz] 2012-05-30 07:03:34 PDT
Mounir, should :disabled match such options?
Comment 2 Mounir Lamouri (:mounir) 2012-05-30 07:05:08 PDT
(In reply to Boris Zbarsky (:bz) from comment #1)
> Mounir, should :disabled match such options?

Yes, definitely.
Comment 3 Boris Zbarsky [:bz] 2012-05-30 07:31:05 PDT
OK, sounds like this should be pretty easy to fix, then...
Comment 4 Mounir Lamouri (:mounir) 2012-05-31 11:50:18 PDT
They should not have their disable state inherited from <select> AFAICT.
Comment 5 Mounir Lamouri (:mounir) 2012-05-31 11:50:45 PDT
Created attachment 628838 [details] [diff] [review]
Patch
Comment 6 Boris Zbarsky [:bz] 2012-05-31 18:32:09 PDT
Comment on attachment 628838 [details] [diff] [review]
Patch

r=me
Comment 7 Boris Zbarsky [:bz] 2012-05-31 18:33:14 PDT
Comment on attachment 628838 [details] [diff] [review]
Patch

Er, I forgot the review comments:

>+++ b/content/html/content/src/nsHTMLOptGroupElement.cpp
>+nsHTMLOptGroupElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
...
>+    // All our children <option> have there :disabled depending on our

s/there/their/ and add "state" after ":disabled".

>+    // disabled attribute. We should make sure there state is updated.

s/there/their/

>+        // No need to call |IsELement()| because it's an HTML element.

"IsElement" (lowercase 'l').
Comment 8 alexander :surkov 2012-05-31 18:41:12 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> They should not have their disable state inherited from <select> AFAICT.

why? they are disabled as well and no difference from user perspective
Comment 9 Mounir Lamouri (:mounir) 2012-06-01 02:38:39 PDT
(In reply to alexander :surkov from comment #8)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> > They should not have their disable state inherited from <select> AFAICT.
> 
> why? they are disabled as well and no difference from user perspective

They are not really disabled, the select is disabled. In a user perspective, that would make no difference because a disabled <select> shouldn't show its options. Is that annoying for some a11y-specific reason?
Comment 10 alexander :surkov 2012-06-01 03:11:59 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #9)
> (In reply to alexander :surkov from comment #8)
> > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> > > They should not have their disable state inherited from <select> AFAICT.
> > 
> > why? they are disabled as well and no difference from user perspective
> 
> They are not really disabled, the select is disabled. In a user perspective,
> that would make no difference because a disabled <select> shouldn't show its
> options. 

> Is that annoying for some a11y-specific reason?

yes. what a11y reports is close to user's perception of UI. If option is not available (it doesn't matter whether its parent control is disabled or the option itself is disabled) then we report that option is not available.

From technical perspective all we do now is checking the disabled state on element assuming it's inherited. If it's not then we should do something like:

if element has not disabled state and if that's a select option then get its parent select element and check its disabled state.

actually we do that for XUL (where iirc element states don't work due to some reason) but I'd be happy to get that for free.
Comment 11 Mounir Lamouri (:mounir) 2012-06-01 05:57:40 PDT
Unfortunately, I don't think there is any reason - that are going to be accepted - to push that to the HTML specifications. I would tend to say that special casing is likely the good solution here. You can make this very simple with the ::GetSelect() method.
Boris, do you see any other solution?
Comment 12 alexander :surkov 2012-06-01 07:04:42 PDT
Nevermind, if HTML shouldn't do that then we handle that on a11y side.
Comment 13 Boris Zbarsky [:bz] 2012-06-01 08:00:24 PDT
> Boris, do you see any other solution?

Not offhand.

Note that a disabled <select multiple> does in fact show the options, so the difference is perceivable via styles on the options... but I agree that from an HTML point of view those options should not be treated as disabled.

Note You need to log in before you can comment on or make changes to this bug.