Web Inspector: AXI: expose computed tree node and heading level AccessibilityNodeObject::headingLevel() AccessibilityNodeObject::hierarchicalLevel()
<rdar://problem/16442349>
Created attachment 290156 [details] Patch
Comment on attachment 290156 [details] Patch Attachment 290156 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2165357 New failing tests: fast/images/pdf-as-image-with-annotations.html
Created attachment 290165 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 290156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290156&action=review Patch looks good, just update the test to actually log the values and include a <h#> element! > Source/WebCore/inspector/InspectorDOMAgent.cpp:1782 > + finalizedHierarchicalLevel = (hierarchicalLevel)? hierarchicalLevel: headingLevel; Style: The ()s are unnecessary and style is to have spaces around operators. finalizedHierarchicalLevel = hierarchicalLevel ? hierarchicalLevel : headingLevel; > LayoutTests/inspector/dom/getAccessibilityPropertiesForNode-expected.txt:26 > role: treeitem > childNodeIds.length: 1 > parentNodeId: exists > + hierarchicalLevel: exists You should add a header element to this page to ensure headingLevel gets tests. You should actually log the hierarchicalLevel. You don't need to say "exists". > LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html:195 > + case "headingLevel": > + case "hierarchicalLevel": I believe these values are fine to log. They don't change every time the test runs. So you should drop these lines and get the "default" cause which logs the values.
Created attachment 290379 [details] Patch
Comment on attachment 290379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290379&action=review > Source/JavaScriptCore/inspector/protocol/DOM.json:93 > + { "name": "headingLevel", "type": "number", "optional": true, "description": "Heading level of a heading element." }, > + { "name": "hierarchicalLevel", "type": "number", "optional": true, "description": "The hierarchical level of an element." }, See comment below about combining these for the wire transfer, then presenting them separately on the client. > Source/WebCore/inspector/InspectorDOMAgent.cpp:1849 > + value->setHeadingLevel(finalizedHierarchicalLevel); What about simply passing "level" over the wire instead of either/or headingLevel/hierarchyLevel? You can always present them with different strings on the client side depending on whether it's a heading. > Source/WebCore/inspector/InspectorDOMAgent.cpp:-1837 > - Avoid standalone whitespace diffs unless it's adjacent to your code changes. > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:397 > +localizedStrings["Hierarchical Level"] = "Hierarchical Level"; What about simplifying to "Hierarchy Level"?
Comment on attachment 290379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290379&action=review >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1849 >> + value->setHeadingLevel(finalizedHierarchicalLevel); > > What about simply passing "level" over the wire instead of either/or headingLevel/hierarchyLevel? You can always present them with different strings on the client side depending on whether it's a heading. Thanks for the comment. Both approach passes only @level over the wire. The only challenge with the suggested approach is that the logic to differentiate Heading Level and Hierarchy Level from a generic string (@level) would have to be reimplemented on the JS side whereas my proposed approach takes advantage of WebCore's logic to make that differentiation.
> Both approach passes only @level over the wire. Since there are different keys for both headingLevel and hierarchicalLevel, perhaps you mean that there is never a time that you send both for the same element? > The only challenge with the suggested approach is that the logic to differentiate Heading Level and Hierarchy Level from a generic string (@level) would have to be reimplemented on the JS side whereas my proposed approach takes advantage of WebCore's logic to make that differentiation. Unless I'm mistaken, the client-side logic would just be: WebInspector.UIString((role == "heading") ? "Heading Level" : "Hierarchy Level")
Created attachment 290633 [details] Patch
(In reply to comment #9) > > Both approach passes only @level over the wire. > > Since there are different keys for both headingLevel and hierarchicalLevel, > perhaps you mean that there is never a time that you send both for the same > element? > Yes, either headingLevel or hierarchicalLevel is set. The schema just has both properties. > > The only challenge with the suggested approach is that the logic to differentiate Heading Level and Hierarchy Level from a generic string (@level) would have to be reimplemented on the JS side whereas my proposed approach takes advantage of WebCore's logic to make that differentiation. > > Unless I'm mistaken, the client-side logic would just be: > > WebInspector.UIString((role == "heading") ? "Heading Level" : "Hierarchy > Level") Yes, it would be a role checking logic, but the role of the element is already inferred when WebsCore provides information about the heading level, so I thought we could save a step there. I also thought that it is cleaner to decouple the the role from the name of the property. It seems more defensive to me to decide what the property should be named based on the availability of the properties.
Created attachment 290672 [details] Patch
updated the related Layout Test.
> Yes, either headingLevel or hierarchicalLevel is set. The schema just has both properties. Okay. I'll defer to the WebKit Inspector team members if they prefer the logic to be on the client or in WebCore. I don't have a strong preference.
(In reply to comment #14) > > Yes, either headingLevel or hierarchicalLevel is set. The schema just has both properties. > > Okay. I'll defer to the WebKit Inspector team members if they prefer the > logic to be on the client or in WebCore. I don't have a strong preference. I have no strong preference here either. Lets leave it for now since it more closely matches the AX objects API.
Comment on attachment 290672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290672&action=review r- for the style issues, but it looks good. Put up one more polished patch and lets land it! > Source/WebCore/inspector/InspectorDOMAgent.cpp:1604 > + auto level = 0; Just make this unsigned. > Source/WebCore/inspector/InspectorDOMAgent.cpp:1853 > + if (headingLevel) > + value->setHeadingLevel(level); > + > + // We do not want it to say Hierarchy Level: 0 > + else if (level) > + value->setHierarchyLevel(level); It is unusual style to have comments between if/else if blocks. Please restyle this so that the code block doesn't have a gap. I'd suggest: // Both comments ... if (headingLevel) value->setHeadingLevel(level); else if (level) value->setHierarchyLevel(level); > LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html:131 > +<h1 class="ex" style="color: red;" aria-level="8">H1</h1> > + > +<ul class="ex" > Some weird whitespace in the attributes. > LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html:143 > + <span>Tree Item Level 1</span> Should these spans be tested? Do they inherit a level? > ManualTests/accessibility/exposing-heading-level.html:1 > +<html> Now that we have automated tests, a Manual Test is probably not needed. Unless you find value in keeping it, it could be removed.
Comment on attachment 290672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290672&action=review >> LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html:143 >> + <span>Tree Item Level 1</span> > > Should these spans be tested? Do they inherit a level? Nope, the <span> does not need to be tested. Just a common way to mark up nested list.
Created attachment 291302 [details] Patch
Comment on attachment 291302 [details] Patch r=me!
Comment on attachment 291302 [details] Patch Rejecting attachment 291302 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 291302, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: /git.webkit.org/WebKit e0aba4b..271d758 master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 207170 = e0aba4be5d9caf05614a10563870de2bf57a0cf4 r207171 = 271d7582ec61f8a815ab88cec656c8fa7692c535 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://webkit-queues.webkit.org/results/2265298
Created attachment 292009 [details] Patch
Comment on attachment 292009 [details] Patch Clearing flags on attachment: 292009 Committed r207553: <http://trac.webkit.org/changeset/207553>
All reviewed patches have been landed. Closing bug.