Bug 104483 – AX: wrong Enabled states on a ListBox

• Component: Accessibility
• Status: RESOLVED FIXED

[Alejandro Piñeiro]

Created attachment 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.

[Alejandro Piñeiro]

Created attachment 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.

[Alejandro Piñeiro]

Chris, could you please review this? As I said is a really easy patch.

[chris fleizach]

Comment on attachment 178417 [details]
Fixes the bug

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

[Alejandro Piñeiro]

Created attachment 178504 [details]
Fixes the bug

(In reply to comment #3)
> (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

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

[WebKit Review Bot]

Comment on attachment 178504 [details]
Fixes the bug

Clearing flags on attachment: 178504

Committed r137168: <http://trac.webkit.org/changeset/137168>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.