Bug 130825 – Web Inspector: AXI: expose computed tree node and heading level

• Component: Web Inspector
• Status: RESOLVED FIXED

[James Craig]

Web Inspector: AXI: expose computed tree node and heading level 

AccessibilityNodeObject::headingLevel()
AccessibilityNodeObject::hierarchicalLevel()

[Radar WebKit Bug Importer]

<rdar://problem/16442349>

[Aaron Chu]

Created attachment 290156 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

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.

[Aaron Chu]

Created attachment 290379 [details]
Patch

[James Craig]

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"?

[Aaron Chu]

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.

[James Craig]

> 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")

[Aaron Chu]

Created attachment 290633 [details]
Patch

[Aaron Chu]

(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.

[Aaron Chu]

Created attachment 290672 [details]
Patch

[Aaron Chu]

updated the related Layout Test.

[James Craig]

> 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.

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

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.

[Aaron Chu]

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.

[Aaron Chu]

Created attachment 291302 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 291302 [details]
Patch

r=me!

[WebKit Commit Bot]

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

[Aaron Chu]

Created attachment 292009 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 292009 [details]
Patch

Clearing flags on attachment: 292009

Committed r207553: <http://trac.webkit.org/changeset/207553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.