Bug 163088 – Web Inspector: AXI: focused/focusable state should be based on Accessibility Object instead of Element

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Aaron Chu]

The current focus implementation uses a DOM focusable check instead of and accessibility focusable check.

[Aaron Chu]

<rdar://problem/16421985>

[Aaron Chu]

Created attachment 290941 [details]
Patch

[Darin Adler]

Comment on attachment 290941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290941&action=review

> Source/WebCore/ChangeLog:13
> +        Covered by existing tests: 
> +        LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html

Maybe the code is exercised by that test, but does that test show the bug? If it did, then there would need to be a test result update in this patch.

[Aaron Chu]

Comment on attachment 290941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290941&action=review

>> Source/WebCore/ChangeLog:13
>> +        LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html
> 
> Maybe the code is exercised by that test, but does that test show the bug? If it did, then there would need to be a test result update in this patch.

I ran the test with the new code and the result was consistant with the result. Would that suffice?

[Joseph Pecoraro]

Comment on attachment 290941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290941&action=review

>>> Source/WebCore/ChangeLog:13
>>> +        LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html
>> 
>> Maybe the code is exercised by that test, but does that test show the bug? If it did, then there would need to be a test result update in this patch.
> 
> I ran the test with the new code and the result was consistant with the result. Would that suffice?

Darin has a point. If this change is fixing an issue, then there should be a test that shows what changed. The existing test didn't change its results, so whatever this patch is fixing is not included in that test! Ideally you would update the test to cover the case that this patch fixes. That will ensure the issue fixed by this patch doesn't regress in the future.

[Joseph Pecoraro]

Comment on attachment 290941 [details]
Patch

From Aaron via email:

> Technically there isn’t an issue, noting was breaking. I think James
> original concern was that the isFocusable was not “authenticatic”
> because it was not coming from an AXObject while we are trying to
> show the related properties in the AX Inspector.

Sounds good to land.

[WebKit Commit Bot]

Comment on attachment 290941 [details]
Patch

Clearing flags on attachment: 290941

Committed r207496: <http://trac.webkit.org/changeset/207496>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.