[webkit-reviews] review granted: [Bug 122574] AX: VoiceOver speaking too much when group elements with tabindex=-1 are used : [Attachment 213830] patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 122574: AX: VoiceOver speaking too much when group elements with
tabindex=-1 are used
https://bugs.webkit.org/show_bug.cgi?id=122574

Attachment 213830: patch
https://bugs.webkit.org/attachment.cgi?id=213830&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=213830&action=review


Looks good to me. I've just made a couple of minor comments for your
consideration before landing.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1325
> +	   useTextUnderElement = true;
> +	   
> +	   // If a node uses a negative tabindex, do not expose it as a generic
focusable element, because keyboard focus management
> +	   // will never land on this specific element.
> +	   Node* node = this->node();
> +	   if (node && node->isElementNode() && toElement(node)->tabIndex() <
0)
> +	       useTextUnderElement = false;

Probably a pointless suggestion, but what about assigning useTextUnderElement
once only? 

e.g: useTextUnderElement = !(node && node->isElementNode() &&
toElement(node)->tabIndex() < 0);

> LayoutTests/accessibility/negative-tabindex-does-not-expose-label.html:18
> +    
> +

One of these empty lines are not needed