Bug 104483 - AX: wrong Enabled states on a ListBox
: AX: wrong Enabled states on a ListBox
Status: RESOLVED FIXED
: WebKit
Accessibility
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-12-09 05:55 PST by
Modified: 2012-12-10 09:31 PST (History)


Attachments
A layout test that should pass but it isn't (2.46 KB, patch)
2012-12-09 05:55 PST, Alejandro Piñeiro
no flags Review Patch | Details | Formatted Diff | Diff
Fixes the bug (5.19 KB, patch)
2012-12-09 07:36 PST, Alejandro Piñeiro
no flags Review Patch | Details | Formatted Diff | Diff
Fixes the bug (5.31 KB, patch)
2012-12-10 03:08 PST, Alejandro Piñeiro
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-12-09 05:55:38 PST
Created an attachment (id=178416) [details]
A layout test that should pass but it isn't

If a ListBox has a disabled item it should be exposed as disabled.

I found this problem while working on the bug 98376 I found a new problem. The bug is about getting the gtk port passing a existing test. On that test the last item is not selectable. But in the same way, it shouldn't be enabled, but it is.

I'm attaching a modified test, with the expected outcome. Right now this is failing. 

Note: I'm reporting this bug to the GTK port, as it is the one I'm working on, but this test should also pass in any port, and I think that this would be failing on any other port.
------- Comment #1 From 2012-12-09 07:36:52 PST -------
Created an attachment (id=178417) [details]
Fixes the bug

The error was at AccessibilityListBoxOption, as it redefined isEnabled, and didn't take into account the DISABLED attribute. So this patch takes that into account, and for completeness sake, I also added a check for aria-disabled. I renamed the test, as this is a listbox specif thing, and also added aria-atribute checks.

As you could have noticed, I also removed the [GTK] prefix at the summary and moved it to the Accessibility component. This is because taking into account the solution, this problem would be affecting any port.
------- Comment #2 From 2012-12-09 08:39:14 PST -------
Chris, could you please review this? As I said is a really easy patch.
------- Comment #3 From 2012-12-09 23:35:57 PST -------
(From update of attachment 178417 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=178417&action=review

It bugs me a bit that we're checking for aria_disabledAttr in two methods, but I see a bunch of different implementations that would make a common implementation a bit tricky and less glamorous to get right. 
I think if we continue to provide custom isEnabled implementations we should think about refactoring a bit, but I think this is OK in current form

just a minor comment about the layout tests

> LayoutTests/accessibility/listbox-enabled-states.html:35
> +        shouldBeTrue('accessibilityController.focusedElement.childAtIndex(4).isEnabled');

you should also include the js-post test scripts so that we get the successfullyParsed messages

> Source/WebCore/ChangeLog:3
> +        Wrong Enabled states on a ListBox

We should probably prefix the bug title with AX: * 
so that others know what this refers to
------- Comment #4 From 2012-12-10 03:08:52 PST -------
Created an attachment (id=178504) [details]
Fixes the bug

(In reply to comment #3)
> (From update of attachment 178417 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178417&action=review
> 
> It bugs me a bit that we're checking for aria_disabledAttr in two methods, but I see a bunch of different implementations that would make a common implementation a bit tricky and less glamorous to get right. 
> I think if we continue to provide custom isEnabled implementations we should think about refactoring a bit, but I think this is OK in current form

I agree.

> just a minor comment about the layout tests
> 
> > LayoutTests/accessibility/listbox-enabled-states.html:35
> > +        shouldBeTrue('accessibilityController.focusedElement.childAtIndex(4).isEnabled');
> 
> you should also include the js-post test scripts so that we get the successfullyParsed messages

Done.

Note: I based this tests on a existing one, so the reasons I didn't notice this. Right now there are several tests that lack the js-post. Anyway not sure if a "cleaning task" it is worth or not.

> 
> > Source/WebCore/ChangeLog:3
> > +        Wrong Enabled states on a ListBox
> 
> We should probably prefix the bug title with AX: * 
> so that others know what this refers to

Done
------- Comment #5 From 2012-12-10 09:31:23 PST -------
(From update of attachment 178504 [details])
Clearing flags on attachment: 178504

Committed r137168: <http://trac.webkit.org/changeset/137168>
------- Comment #6 From 2012-12-10 09:31:26 PST -------
All reviewed patches have been landed.  Closing bug.