Bug 129779 – Web Inspector: AXI: Expose focused/focusable state in the Accessibility Node Inspector

• Component: Web Inspector
• Status: RESOLVED FIXED

[James Craig]

Web Inspector: AXI: Expose focused/focusable state in the Accessibility Node Inspector

[James Craig]

<rdar://problem/16245334>

[Diego Pino]

Created attachment 226113 [details]
Patch

[James Craig]

If a bug is already assigned, please contact the assignee before taking it for a patch. I hadn't done any work on this yet, but we could have duplicated efforts. Reassigning this one to you for the time being. 

R- because the utility needed here is whether an element is focusable, not just focused. You'll want to add a "supportsFocus(ed)" or "focusable" bool in InspectorDOMAgent.cpp that gets assigned if the element is focusable, and only assign send back the focused value if it's focusable. Then use the booleanValueToLocalizedStringIfPropertyDefined("focused") helper function (coming in patch for bug 129781) and display either value in the client as long as it's not undefined. Note: there may be some merge conflicts with bug 129781, too. Hopefully that doesn't slow you down too much. Thanks for starting on this, just let me know next time if it's already assigned.

Please update the Layout Test, too: inspector-protocol/dom/getAccessibilityPropertiesForNode.html

[James Craig]

I'm getting site errors when attempting to r- and cq-. This is just a note to reviewers to not r+ this one yet.

[Joseph Pecoraro]

Comment on attachment 226113 [details]
Patch

r- based on James Craig's comments.

[Diego Pino]

(In reply to comment #3)
> If a bug is already assigned, please contact the assignee before taking it for a patch. I hadn't done any work on this yet, but we could have duplicated efforts. Reassigning this one to you for the time being. 
> 

Sorry, I didn't realise it was assigned. Completely agree with your point of view.

> R- because the utility needed here is whether an element is focusable, not just focused. You'll want to add a "supportsFocus(ed)" or "focusable" bool in InspectorDOMAgent.cpp that gets assigned if the element is focusable, and only assign send back the focused value if it's focusable. Then use the booleanValueToLocalizedStringIfPropertyDefined("focused") helper function (coming in patch for bug 129781) and display either value in the client as long as it's not undefined. Note: there may be some merge conflicts with bug 129781, too. Hopefully that doesn't slow you down too much. Thanks for starting on this, just let me know next time if it's already assigned.

OK, I'll redo the patch according to those instructions. Thanks for the explanation.

> 
> Please update the Layout Test, too: inspector-protocol/dom/getAccessibilityPropertiesForNode.html

OK.

[Diego Pino]

Created attachment 226486 [details]
Patch

[James Craig]

Comment on attachment 226486 [details]
Patch

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

The rest looks okay to me.

> Source/WebCore/inspector/protocol/DOM.json:66
> +                { "name": "focused", "type": "boolean", "optional": true, "description": "Focused state." },

"Focused state. Only defined on focusable elements."

[Diego Pino]

Created attachment 226491 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 226491 [details]
Patch

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

> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode-expected.txt:91
>      exists: true
>      label: 
>      role: 
> +    focused: false
>      ignored: true
>      ignoredByDefault: true
>      hidden: true

The tests all have focused: false. Would be nice to include a test where focused is true. Would it be sufficient to just do inputElement.focus() in the test?

r=me

[Joseph Pecoraro]

Let me know if you need me to cq+

[James Craig]

Comment on attachment 226491 [details]
Patch

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

>> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode-expected.txt:91
>>      hidden: true
> 
> The tests all have focused: false. Would be nice to include a test where focused is true. Would it be sufficient to just do inputElement.focus() in the test?
> 
> r=me

That'd be good, but I don't know if it's necessary for this one. I think we'll want a second test (in a later bug, not this one) to test all the iterations of focused/focusable on various elements.

[Diego Pino]

(In reply to comment #11)
> Let me know if you need me to cq+

Yes please :)

[James Craig]

Just an FYI that you'll need to post another conflict resolution due to the patch for bug 130037. Normally this would not have been a problem, but the commit bot has been down this week.

[Diego Pino]

Created attachment 226593 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 226593 [details]
Patch

Clearing flags on attachment: 226593

Committed r165590: <http://trac.webkit.org/changeset/165590>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.