Bug 130721 – Web Inspector: AXI: expose what elements get generic "clickable" status

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[James Craig]

Web Inspector: AXI: expose what elements get generic "clickable" status

If an element has a mousedown/mouseup/click handler but is not one of the types that users expect to be clickable (button, link, etc.) VoiceOver will speak "clickable"… I think we can expose that in the Inspector for authoring and validation purposes. Potential error or warning may be that ~"Element has a click handler but is not a role that users expect to be clickable." It also might be problematic to use a role whitelist here. Perhaps a blacklist: unknowns, containers, etc.

AccessibilityNodeObject::mouseButtonListener()

[Radar WebKit Bug Importer]

<rdar://problem/16419907>

[James Craig]

Created attachment 227920 [details]
patch

[James Craig]

Comment on attachment 227920 [details]
patch

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

Caught a few minor things after posting. Will put these changes in a patch along with any other feedback.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1053
> +        // If we've reached the body and this is not a control element, do not expose press action for this element unless includeBodyElement is true.

I changed the code but forgot to update the comment. Needs to be: "…unless ignoreBodyElement is false."

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1058
> +        if (ignoreBodyElement) {
> +            if (element.hasTagName(bodyTag) && isStaticText())
> +                break;
> +        }

This should probably be a single-line conditional.

`if (ignoreBodyElement && element.hasTagName(bodyTag) && isStaticText())`

[chris fleizach]

Comment on attachment 227920 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1055
> +        if (ignoreBodyElement) {

looks like this can be one if line
if (ignoreBody && element.hasTagName && isStaticText()

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1063
> +        // Even if ignoreBodyElement is false, don't go higher in the DOM.

The only other element would be the HTML element. is it a problem to go up to that element and then return?
Can you add why we don't want to go up any higher rather than stating what the code is doing

> Source/WebCore/accessibility/AccessibilityNodeObject.h:134
> +    Element* mouseButtonListener(bool ignoreBodyElement) const;

you can set the default value to true with

Element* mouseButtonListener(bool ignoreBodyElement = true)

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1293
> +        return !toRenderBlockFlow(m_renderer)->hasLines() && !mouseButtonListener(true);

if the default value  is set to true, you won't need to check this line

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:271
> +localizedStrings["Mouse Event"] = "Mouse Event";

Mouse event seems a bit vague. Would
"Click Handler" be better

[Joseph Pecoraro]

Comment on attachment 227920 [details]
patch

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

Inspector parts look good. Nice test!

>> Source/WebCore/accessibility/AccessibilityNodeObject.h:134
>> +    Element* mouseButtonListener(bool ignoreBodyElement) const;
> 
> you can set the default value to true with
> 
> Element* mouseButtonListener(bool ignoreBodyElement = true)

We tend to prefer enum values instead of single bool arguments, that way call sites can be more readable. For example: mouseButtonListener(IgnoreBodyElement) is more readable then mouseButtonListener(true).

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:271
>> +localizedStrings["Mouse Event"] = "Mouse Event";
> 
> Mouse event seems a bit vague. Would
> "Click Handler" be better

I like that as well.

[James Craig]

Comment on attachment 227920 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1055
>> +        if (ignoreBodyElement) {
> 
> looks like this can be one if line
> if (ignoreBody && element.hasTagName && isStaticText()

Yes. Noted that same thing below.

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1063
>> +        // Even if ignoreBodyElement is false, don't go higher in the DOM.
> 
> The only other element would be the HTML element. is it a problem to go up to that element and then return?
> Can you add why we don't want to go up any higher rather than stating what the code is doing

I suppose I can remove this. I don't know of any case where people use listeners on the root element. Joe, do you?

>>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:271
>>> +localizedStrings["Mouse Event"] = "Mouse Event";
>> 
>> Mouse event seems a bit vague. Would
>> "Click Handler" be better
> 
> I like that as well.

"Click Handler" is somewhat misleading b/c this is true for click, mousedown, and mouseup. I had "Mouse handler" here when developing. Is your point that this doesn't clearly indicate that this would be fired on touch screen tap events, too? "Handler" usually refers to the function that is called, not the element where the listener is. 

I think I'm going to try changing the label based on whether the element itself has the listener or a delegate element does.

Click Hander: Yes (instead of Mouse Event: Self)
Click Delegate: <node> (instead of Mouse Event: <node>)

[Timothy Hatcher]

Comment on attachment 227920 [details]
patch

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

>>>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:271
>>>> +localizedStrings["Mouse Event"] = "Mouse Event";
>>> 
>>> Mouse event seems a bit vague. Would
>>> "Click Handler" be better
>> 
>> I like that as well.
> 
> "Click Handler" is somewhat misleading b/c this is true for click, mousedown, and mouseup. I had "Mouse handler" here when developing. Is your point that this doesn't clearly indicate that this would be fired on touch screen tap events, too? "Handler" usually refers to the function that is called, not the element where the listener is. 
> 
> I think I'm going to try changing the label based on whether the element itself has the listener or a delegate element does.
> 
> Click Hander: Yes (instead of Mouse Event: Self)
> Click Delegate: <node> (instead of Mouse Event: <node>)

What about Clickable: Yes/No and Click Listener: <node> (or Click Element)? Handler and Delegate are pretty synonymous and could be confusing. They are also not typical words uses by web developers.

[James Craig]

Created attachment 227967 [details]
patch with review feedback

[WebKit Commit Bot]

Attachment 227967 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AccessibilityNodeObject.h:139:  The parameter name "filter" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Craig]

Created attachment 227968 [details]
patch

[Build Bot]

Comment on attachment 227968 [details]
patch

Attachment 227968 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4742628079828992

New failing tests:
compositing/columns/composited-rl-paginated-repaint.html

[Build Bot]

Created attachment 227972 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[James Craig]

That test failure is unrelated. cq?

[James Craig]

Comment on attachment 227968 [details]
patch

Actually that may be wrong. Investigating further.

[James Craig]

Yeah, it's just a flaky test. I'm getting passes and failures with or without the patch. cq?

[Timothy Hatcher]

Comment on attachment 227968 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1053
> +        // If we've reached the body and this is not a control element, do not expose press action for this element unless resultFilter is ignoreBodyElement.

IncludeBodyElement?

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1532
> +                MouseButtonListenerResultFilter filter = IncludeBodyElement;

Can be inline, no need for the var.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:364
> +                        mouseEventTextValue = WebInspector.UIString("Yes");

Could just hide this row if it is the same node. Having Clickable: Yes, Click Listener: Yes isn't that useful.

[James Craig]

Comment on attachment 227968 [details]
patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:364
>> +                        mouseEventTextValue = WebInspector.UIString("Yes");
> 
> Could just hide this row if it is the same node. Having Clickable: Yes, Click Listener: Yes isn't that useful.

There's only one row. It's blank of there is no click mouse event. If there is an event, it displays either "Clickable: Yes" or "Click Listener: <linked ancestor node>". There's never "Click Listener: Yes."

[James Craig]

Created attachment 228026 [details]
patch with review feedback

[WebKit Commit Bot]

Comment on attachment 228026 [details]
patch with review feedback

Rejecting attachment 228026 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 228026, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
tAccessibilityPropertiesForNode.html
Hunk #1 succeeded at 104 (offset 6 lines).
Hunk #2 succeeded at 176 (offset 6 lines).
patching file LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode_mouseEventNodeId-expected.txt
patching file LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode_mouseEventNodeId.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/6707649169063936

[James Craig]

Re-baselining the test conflict from TOT.

[James Craig]

Created attachment 228086 [details]
rebaselined expectation

[WebKit Commit Bot]

Comment on attachment 228086 [details]
rebaselined expectation

Clearing flags on attachment: 228086

Committed r166438: <http://trac.webkit.org/changeset/166438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.