Web Inspector: AXI: Expose focused/focusable state in the Accessibility Node Inspector
<rdar://problem/16245334>
Created attachment 226113 [details] Patch
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
I'm getting site errors when attempting to r- and cq-. This is just a note to reviewers to not r+ this one yet.
Comment on attachment 226113 [details] Patch r- based on James Craig's comments.
(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.
Created attachment 226486 [details] Patch
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."
Created attachment 226491 [details] Patch
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
Let me know if you need me to cq+
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.
(In reply to comment #11) > Let me know if you need me to cq+ Yes please :)
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.
Created attachment 226593 [details] Patch
Comment on attachment 226593 [details] Patch Clearing flags on attachment: 226593 Committed r165590: <http://trac.webkit.org/changeset/165590>
All reviewed patches have been landed. Closing bug.