Created attachment 216983 [details] test case demonstrating bugs AX: Improve ARIA live region reliability by sending notifications when live regions are created/shown and hidden/destroyed
<rdar://problem/15124143>
*** Bug 62289 has been marked as a duplicate of this bug. ***
*** Bug 118541 has been marked as a duplicate of this bug. ***
Created attachment 228345 [details] patch
Created attachment 228348 [details] patch
Comment on attachment 228348 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=228348&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:1486 > +const String AccessibilityObject::defaultLiveRegionStatusForRole(AccessibilityRole role) Wouldn't it be better to return a reference to a statically stored AtomicString ("const AtomicString&" return type)? > Source/WebCore/accessibility/AccessibilityObject.cpp:1840 > + return equalIgnoringCase(liveRegionStatus, "polite") || equalIgnoringCase(liveRegionStatus, "assertive"); You could check instead for !liveRegionStatus.isNull() && !equalIgnoringCase(liveRegionStatus, "off", although the end result would be the same of course (http://www.w3.org/TR/wai-aria/states_and_properties#aria-live). Just commenting it because in my mind sounds better to exclude the negatives than include all the positive, but I'd rather leave it up to you :) > Source/WebCore/accessibility/AccessibilityObject.h:839 > - virtual const AtomicString& ariaLiveRegionStatus() const { return nullAtom; } > + virtual const String ariaLiveRegionStatus() const { return String(); } Following the same reasoning than in my previous comment: is there any specific reason why you're changing the return type from AtomicString& to String? By checking the new code I still get the feeling that returning a reference to a statically stored AtomicString would still be a good idea > LayoutTests/platform/mac/accessibility/live-region-creation-notification.html:38 > + window.setTimeout(function() { appendLiveRegion('third'); }, t*1); > + > + // add fourth live region (hidden), then show later > + window.setTimeout(function() { appendLiveRegion('fourth', HIDE); }, t*2); > + window.setTimeout(function() { unhide('fourth'); }, t*3); > + window.setTimeout(function() { appendDiv('fifth'); }, t*4); > + > + // update them sequentially after all have been loaded > + window.setTimeout(updateAll, t*5); I'm a bit concerned about chaining the executions of this functions using timeouts (and so small ones), in terms of whether this could cause some flakiness. Will it be possible to chaining them using a continuation passing style pattern, instead of the timeouts? > LayoutTests/platform/mac/accessibility/live-region-creation-notification.html:51 > + window.setTimeout(function() { updateLiveRegion('first'); }, t*1); > + window.setTimeout(function() { updateLiveRegion('second'); }, t*2); > + window.setTimeout(function() { updateLiveRegion('third'); }, t*3); > + window.setTimeout(function() { updateLiveRegion('fourth'); }, t*4); > + window.setTimeout(function() { endTest(); }, t*5); Same comment here: if possible, I'd say that using CPS here would be safer
(In reply to comment #6) > (From update of attachment 228348 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228348&action=review > > > Source/WebCore/accessibility/AccessibilityObject.cpp:1486 > > +const String AccessibilityObject::defaultLiveRegionStatusForRole(AccessibilityRole role) > > Wouldn't it be better to return a reference to a statically stored AtomicString ("const AtomicString&" return type)? I removed the static strings in ariaLiveRegionStatus (for off, assertive, polite) because of the desire made by others in WebKit that we don't use NeverDestroyed strings in areas where performance is not super critical. By removing that I no longer had a reference I could pass back (instead I was used ASCIILiteral), so a few of these methods had to change to String() as a result. I think that's the direction that people want these static strings to go in, so it seemed to make sense to me to change it here. > > > Source/WebCore/accessibility/AccessibilityObject.cpp:1840 > > + return equalIgnoringCase(liveRegionStatus, "polite") || equalIgnoringCase(liveRegionStatus, "assertive"); > > You could check instead for !liveRegionStatus.isNull() && !equalIgnoringCase(liveRegionStatus, "off", although the end result would be the same of course (http://www.w3.org/TR/wai-aria/states_and_properties#aria-live). Just commenting it because in my mind sounds better to exclude the negatives than include all the positive, but I'd rather leave it up to you :) I think it might be better to check for positives here because if you set your aria-live="blah" we don't want that to register as a live region enabled. > > > Source/WebCore/accessibility/AccessibilityObject.h:839 > > - virtual const AtomicString& ariaLiveRegionStatus() const { return nullAtom; } > > + virtual const String ariaLiveRegionStatus() const { return String(); } > > Following the same reasoning than in my previous comment: is there any specific reason why you're changing the return type from AtomicString& to String? By checking the new code I still get the feeling that returning a reference to a statically stored AtomicString would still be a good idea > > > LayoutTests/platform/mac/accessibility/live-region-creation-notification.html:38 > > + window.setTimeout(function() { appendLiveRegion('third'); }, t*1); > > + > > + // add fourth live region (hidden), then show later > > + window.setTimeout(function() { appendLiveRegion('fourth', HIDE); }, t*2); > > + window.setTimeout(function() { unhide('fourth'); }, t*3); > > + window.setTimeout(function() { appendDiv('fifth'); }, t*4); > > + > > + // update them sequentially after all have been loaded > > + window.setTimeout(updateAll, t*5); > > I'm a bit concerned about chaining the executions of this functions using timeouts (and so small ones), in terms of whether this could cause some flakiness. > > Will it be possible to chaining them using a continuation passing style pattern, instead of the timeouts? > > > LayoutTests/platform/mac/accessibility/live-region-creation-notification.html:51 > > + window.setTimeout(function() { updateLiveRegion('first'); }, t*1); > > + window.setTimeout(function() { updateLiveRegion('second'); }, t*2); > > + window.setTimeout(function() { updateLiveRegion('third'); }, t*3); > > + window.setTimeout(function() { updateLiveRegion('fourth'); }, t*4); > > + window.setTimeout(function() { endTest(); }, t*5); > > Same comment here: if possible, I'd say that using CPS here would be safer Will do
Comment on attachment 228348 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=228348&action=review Setting r+ after your comments, but please consider the comments >>> Source/WebCore/accessibility/AccessibilityObject.cpp:1486 >>> +const String AccessibilityObject::defaultLiveRegionStatusForRole(AccessibilityRole role) >> >> Wouldn't it be better to return a reference to a statically stored AtomicString ("const AtomicString&" return type)? > > I removed the static strings in ariaLiveRegionStatus (for off, assertive, polite) because of the desire made by others in WebKit that we don't use NeverDestroyed strings in areas where performance is not super critical. By removing that I no longer had a reference I could pass back (instead I was used ASCIILiteral), so a few of these methods had to change to String() as a result. > > I think that's the direction that people want these static strings to go in, so it seemed to make sense to me to change it here. Fair enough :) >>> Source/WebCore/accessibility/AccessibilityObject.cpp:1840 >>> + return equalIgnoringCase(liveRegionStatus, "polite") || equalIgnoringCase(liveRegionStatus, "assertive"); >> >> You could check instead for !liveRegionStatus.isNull() && !equalIgnoringCase(liveRegionStatus, "off", although the end result would be the same of course (http://www.w3.org/TR/wai-aria/states_and_properties#aria-live). Just commenting it because in my mind sounds better to exclude the negatives than include all the positive, but I'd rather leave it up to you :) > > I think it might be better to check for positives here because if you set your aria-live="blah" we don't want that to register as a live region enabled. Fair enough too
http://trac.webkit.org/changeset/166649 Made the changes for the layout test. Thanks Mario
(In reply to comment #9) > http://trac.webkit.org/changeset/166649 > > Made the changes for the layout test. Thanks Mario I think test results may be missing for LayoutTests/platform/mac/accessibility/live-region-creation-notification.html.
Checked in http://trac.webkit.org/changeset/166671
(In reply to comment #9) > http://trac.webkit.org/changeset/166649 > > Made the changes for the layout test. Thanks Mario No problem, and sorry for the noisy review :)
*** Bug 132662 has been marked as a duplicate of this bug. ***