[webkit-reviews] review requested: [Bug 104483] AX: wrong Enabled states on a ListBox : [Attachment 178504] Fixes the bug

[bugzilla-daemon at webkit.org]

Alejandro Piñeiro <apinheiro at igalia.com> has asked  for review:
Bug 104483: AX: wrong Enabled states on a ListBox
https://bugs.webkit.org/show_bug.cgi?id=104483

Attachment 178504: Fixes the bug
https://bugs.webkit.org/attachment.cgi?id=178504&action=review

------- Additional Comments from Alejandro Piñeiro <apinheiro at igalia.com>
(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