Bug 163088 - Web Inspector: AXI: focused/focusable state should be based on Accessibility Object instead of Element
Summary: Web Inspector: AXI: focused/focusable state should be based on Accessibility ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: Safari 10
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-06 14:30 PDT by Aaron Chu
Modified: 2016-10-18 16:07 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.78 KB, patch)
2016-10-07 10:41 PDT, Aaron Chu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Chu 2016-10-06 14:30:11 PDT
The current focus implementation uses a DOM focusable check instead of and accessibility focusable check.
Comment 1 Aaron Chu 2016-10-06 14:33:09 PDT
<rdar://problem/16421985>
Comment 2 Aaron Chu 2016-10-07 10:41:24 PDT
Created attachment 290941 [details]
Patch
Comment 3 Darin Adler 2016-10-07 12:21:11 PDT
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.
Comment 4 Aaron Chu 2016-10-07 12:31:02 PDT
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?
Comment 5 Joseph Pecoraro 2016-10-11 12:07:00 PDT
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.
Comment 6 Joseph Pecoraro 2016-10-18 15:43:34 PDT
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.
Comment 7 WebKit Commit Bot 2016-10-18 16:06:56 PDT
Comment on attachment 290941 [details]
Patch

Clearing flags on attachment: 290941

Committed r207496: <http://trac.webkit.org/changeset/207496>
Comment 8 WebKit Commit Bot 2016-10-18 16:07:00 PDT
All reviewed patches have been landed.  Closing bug.