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": "..." } ] }, ] }
<rdar://problem/16462430>
<rdar://problem/16462429>
UI can probably use the same row as the live region status/priority. Live: Polite (Additions, Text) Live: Assertive (Deletions)
(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.
Created attachment 233020 [details] patch
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>
> Is it basically doing: > > var stringifyA = a.join(","); > var stringifyB = a.join(","); > return stringilyA === stringifyB; Err of course I meant, stringifyB being b.join(","). =)
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.
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) { ... });
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.
(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.
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>'
Created attachment 233282 [details] patch with review feedback
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)
Created attachment 233317 [details] patch with review feedback
Not sure about that build failure. Baselining to see if that resolves it.
Created attachment 233338 [details] patch
"c++: internal compiler error: Killed (program cc1plus)", not sure what you can do about that.
Yeah, seems to have just been a fluke.
Comment on attachment 233338 [details] patch Clearing flags on attachment: 233338 Committed r170138: <http://trac.webkit.org/changeset/170138>
All reviewed patches have been landed. Closing bug.