[webkit-reviews] review granted: [Bug 130913] Web Inspector: AXI: expose aria-relevant : [Attachment 233020] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted James Craig
<jcraig at apple.com>'s request for review:
Bug 130913: Web Inspector: AXI: expose aria-relevant
https://bugs.webkit.org/show_bug.cgi?id=130913

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

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=233020&action=review


Looks good. Some possible improvements.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1559
> +		   if (ariaRelevantAttrValue != "") {

Nit: !ariaRelevantAttrValue.isEmpty()

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1560
> +		       

Style: Unnecessary empty line.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1573
> +			   liveRegionRelevant->addItem(ariaRelevantAdditions);
> +			   liveRegionRelevant->addItem(ariaRelevantRemovals);
> +			   liveRegionRelevant->addItem(ariaRelevantText);

There is an enum value "All". Should we just use that? (ariaRelevantAll)

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1669
> +	       if (liveRegionRelevant->length())

Nit: !liveRegionRelevant->isEmpty()

> Source/WebCore/inspector/protocol/DOM.json:18
> +	       "enum": ["additions", "all", "removals", "text"],

Currently "all" is unused.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:269
> +	   function checkEqualityOfArrays(a, b) {
> +	       return !(a < b || b < a);
> +	   }

This is somewhat misleading.

Is it basically doing:

    var stringifyA = a.join(",");
    var stringifyB = a.join(",");
    return stringilyA === stringifyB;

I think we may be able to avoid this function.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:386
> +			   var allRelevant =
[DOMAgent.LiveRegionRelevant.Additions, DOMAgent.LiveRegionRelevant.Removals,
DOMAgent.LiveRegionRelevant.Text];
> +			   if (checkEqualityOfArrays(liveRegionRelevant,
allRelevant))
> +			       liveRegionRelevant = [WebInspector.UIString("All
Changes")];

Should we just check for DOMAgent.LiveRegionRelevant.All, and if so avoid the
brittle array equality check?

I see you would still need to handle the implicit case. We could cheat, avoid
duplicates, and just check if the array length === 3 for all right now.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:402
> +			       for (index in liveRegionRelevant) {
> +				   switch (liveRegionRelevant[index]) {
> +				   case DOMAgent.LiveRegionRelevant.Additions:
> +				       liveRegionRelevant[index] =
WebInspector.UIString("Additions");
> +				       break;
> +				   case DOMAgent.LiveRegionRelevant.Removals:
> +				       liveRegionRelevant[index] =
WebInspector.UIString("Removals");
> +				       break;
> +				   case DOMAgent.LiveRegionRelevant.Text:
> +				       liveRegionRelevant[index] =
WebInspector.UIString("Text");
> +				       break;
> +				   default: // If WebCore sends a new unhandled
value, display as a String.
> +				       liveRegionRelevant[index] = "\"" +
liveRegionRelevant[index] + "\""; 
> +				   }
> +			       }

liveRegionRelevant is an array. We prefer looping over arrays with non for..in
loops. There is the traditional for loop, and for..of. (Also, in this case you
would need "var index" to avoid leaking the index variable to global scope).

But what you are really doing is replacing the elements in liveRegionRelevant,
and for that there is Array.prototype.map:
<https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objec
ts/Array/map>

I'd suggest:

    liveRegionRelevant.map(function(value) {
	switch (value) {
	case DOMAgent.LiveRegionRelevant.Additions:
	    return WebInspector.UIString("Additions");
	case DOMAgent.LiveRegionRelevant.Removals:
	    return WebInspector.UIString("Removals");
	case DOMAgent.LiveRegionRelevant.Text:
	    return WebInspector.UIString("Text");
	default: // If WebCore sends a new unhandled value, display as a
String.
	    return "\"" + value + "\"";
	}
    });

>
LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode_liveRegion
.html:41
> +    <div class="ex" role="alert" aria-relevant="all">relevant: all
(explicit)</div>
> +    <div class="ex" role="alert" aria-relevant="additions removals
text">relevant: all (implicit)</div>
> +    <div class="ex" role="alert" aria-relevant="text additions
removals">relevant: all (implicit)</div>
> +    <div class="ex" role="alert" aria-relevant="text removals
additions">relevant: all (implicit)</div>

Excellent tests. Can we also test a duplicate? I wonder if SpaceSplitString
handles duplicates gracefully or if we will have to:

    <div class="ex" role="alert" aria-relevant="additions removals
removals">relevant: additions removals (duplicate)</div>