Bug 130913 – Web Inspector: AXI: expose aria-relevant

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

[James Craig]

AccessibilityObject::ariaLiveRegionRelevant()

Will also need a new structure for the inspector-protocol like this:
{
    "domain": "DOM",
    "types": [
        {
            "id": "LiveRegionRelevance",
            "type": "string",
            "enum": ["additions", "all", "removals", "text", "none"],
            "description": "...."
        }
    ],
    "commands": [
        {
            "name": "...",
            "parameters": [
                { "name": "paramName", "type": "array", "items": { "$ref": "LiveRegionRelevance" }, "description": "..." }
            ]
        },
    ]
}

[Radar WebKit Bug Importer]

<rdar://problem/16462430>

[Radar WebKit Bug Importer]

<rdar://problem/16462429>

[James Craig]

UI can probably use the same row as the live region status/priority.

Live: Polite (Additions, Text)
Live: Assertive (Deletions)

[Timothy Hatcher]

(In reply to comment #3)
> UI can probably use the same row as the live region status/priority.
> 
> Live: Polite (Additions, Text)
> Live: Assertive (Deletions)

I like that idea.

[James Craig]

Created attachment 233020 [details]
patch

[Joseph Pecoraro]

Comment on attachment 233020 [details]
patch

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_Objects/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>

[Joseph Pecoraro]

> Is it basically doing:
> 
>     var stringifyA = a.join(",");
>     var stringifyB = a.join(",");
>     return stringilyA === stringifyB;


Err of course I meant, stringifyB being b.join(","). =)

[Joseph Pecoraro]

Comment on attachment 233020 [details]
patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1580
> +                        if (values.contains(ariaRelevantAdditions))
> +                            liveRegionRelevant->addItem(ariaRelevantAdditions);
> +                        if (values.contains(ariaRelevantRemovals))
> +                            liveRegionRelevant->addItem(ariaRelevantRemovals);
> +                        if (values.contains(ariaRelevantText))
> +                            liveRegionRelevant->addItem(ariaRelevantText);

Likewise, if all of these trigger, maybe we could just send "All", to simplify things on the frontend side.

[Joseph Pecoraro]

Comment on attachment 233020 [details]
patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:402
>> +                            }
> 
> 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_Objects/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 + "\"";
>         }
>     });

Correction, forgot the assignment, since map creates a new array:

    liveRegionRelevant = liveRegionRelevant.map(function(value) { ... });

[James Craig]

Comment on attachment 233020 [details]
patch

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

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

Thanks. Is there an execution speed difference or is this just for readability?

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1560
>> +                    
> 
> Style: Unnecessary empty line.

Ok.

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1573
>> +                        liveRegionRelevant->addItem(ariaRelevantText);
> 
> There is an enum value "All". Should we just use that? (ariaRelevantAll)

I went back and forth on this. I went this way for clarity in the inspector-protocol. Since this is not exposed to the UI, I'd rather not have ["all"] mean exactly the same thing as ["additions", "removals", "text"], especially since the definition of what constitutes "all" may change in the future, and we'd still need to do the extra work on the client-side anyway. Aside: I personally think @aria-relevant is one of the problematic and under-defined parts of ARIA 1.0 that I hope we can fix in later releases.

I'm leaning toward just removing the All enum.

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

Thanks.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:386
>> +                            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.

That seems more brittle. If WebCore is updated to accept a new value, it could potentially return ["additions", "text", "foo"] and display as All in the UI. Would you be more comfortable with a more explicit check such as this? Since we control the I/O of this on both sides, it's not any more or less brittle either way.

if (liveRegionRelevant === 3 
    && liveRegionRelevant[0] === DOMAgent.LiveRegionRelevant.Additions
    && liveRegionRelevant[1] === DOMAgent.LiveRegionRelevant.Removals
    && liveRegionRelevant[2] === DOMAgent.LiveRegionRelevant.Text)

>>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:402
>>> +                            }
>> 
>> 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_Objects/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 + "\"";
>>         }
>>     });
> 
> Correction, forgot the assignment, since map creates a new array:
> 
>     liveRegionRelevant = liveRegionRelevant.map(function(value) { ... });

The reason I avoided for..of was because it gave me a copy of the array value, but here I needed to reassign it in place. I'll try your map suggestion.

>> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode_liveRegion.html:41
>> +    <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>

Good idea. I think the accessibility code will rip out the dupe before it gets to SpaceSplitString here, but it won't hurt to check.

[Joseph Pecoraro]

(In reply to comment #10)
> (From update of attachment 233020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233020&action=review
> 
> >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1559
> >> +                if (ariaRelevantAttrValue != "") {
> > 
> > Nit: !ariaRelevantAttrValue.isEmpty()
> 
> Thanks. Is there an execution speed difference or is this just for readability?

Just readability. There may be an execution difference but it is probably negligible here.


> >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1573
> >> +                        liveRegionRelevant->addItem(ariaRelevantText);
> > 
> > There is an enum value "All". Should we just use that? (ariaRelevantAll)
> 
> I went back and forth on this. I went this way for clarity in the inspector-protocol. Since this is not exposed to the UI, I'd rather not have ["all"] mean exactly the same thing as ["additions", "removals", "text"], especially since the definition of what constitutes "all" may change in the future, and we'd still need to do the extra work on the client-side anyway. Aside: I personally think @aria-relevant is one of the problematic and under-defined parts of ARIA 1.0 that I hope we can fix in later releases.
> 
> I'm leaning toward just removing the All enum.

Okay.


> >> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:386
> >> +                            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.
> 
> That seems more brittle. If WebCore is updated to accept a new value, it could potentially return ["additions", "text", "foo"] and display as All in the UI. Would you be more comfortable with a more explicit check such as this? Since we control the I/O of this on both sides, it's not any more or less brittle either way.
> 
> if (liveRegionRelevant === 3 
>     && liveRegionRelevant[0] === DOMAgent.LiveRegionRelevant.Additions
>     && liveRegionRelevant[1] === DOMAgent.LiveRegionRelevant.Removals
>     && liveRegionRelevant[2] === DOMAgent.LiveRegionRelevant.Text)

This sounds good if we can guarantee order.

[James Craig]

Comment on attachment 233020 [details]
patch

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

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

error: no member named 'isEmpty' in 'Inspector::TypeBuilder::Array<WTF::String>'

[James Craig]

Created attachment 233282 [details]
patch with review feedback

[Joseph Pecoraro]

Comment on attachment 233282 [details]
patch with review feedback

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

Awesome! r=me

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:402
> +                        // Token values: No need to localize the string concatenation.

I don't think this comment is needed.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:406
> +                    if (accessibilityProperties.liveRegionAtomic === true) {

Nit: No need for the "=== true" (I realize this is old code)

[James Craig]

Created attachment 233317 [details]
patch with review feedback

[James Craig]

Not sure about that build failure. Baselining to see if that resolves it.

[James Craig]

Created attachment 233338 [details]
patch

[Joseph Pecoraro]

"c++: internal compiler error: Killed (program cc1plus)", not sure what you can do about that.

[James Craig]

Yeah, seems to have just been a fluke.

[WebKit Commit Bot]

Comment on attachment 233338 [details]
patch

Clearing flags on attachment: 233338

Committed r170138: <http://trac.webkit.org/changeset/170138>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.