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()
<rdar://problem/16419907>
Created attachment 227920 [details] patch
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())`
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
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.
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>)
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.
Created attachment 227967 [details] patch with review feedback
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.
Created attachment 227968 [details] patch
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
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
That test failure is unrelated. cq?
Comment on attachment 227968 [details] patch Actually that may be wrong. Investigating further.
Yeah, it's just a flaky test. I'm getting passes and failures with or without the patch. cq?
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.
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."
Created attachment 228026 [details] patch with review feedback
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
Re-baselining the test conflict from TOT.
Created attachment 228086 [details] rebaselined expectation
Comment on attachment 228086 [details] rebaselined expectation Clearing flags on attachment: 228086 Committed r166438: <http://trac.webkit.org/changeset/166438>
All reviewed patches have been landed. Closing bug.