Bug 124381 – AX: Improve ARIA live region reliability by sending notifications when live regions are created/shown and hidden/destroyed

• Component: Accessibility
• Status: RESOLVED FIXED

[James Craig]

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

[James Craig]

<rdar://problem/15124143>

[James Craig]

*** Bug 62289 has been marked as a duplicate of this bug. ***

[James Craig]

*** Bug 118541 has been marked as a duplicate of this bug. ***

[chris fleizach]

Created attachment 228345 [details]
patch

[chris fleizach]

Created attachment 228348 [details]
patch

[Mario Sanchez Prada]

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

[chris fleizach]

(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

[Mario Sanchez Prada]

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

[chris fleizach]

http://trac.webkit.org/changeset/166649

Made the changes for the layout test. Thanks Mario

[David Kilzer (:ddkilzer)]

(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.

[chris fleizach]

Checked in
http://trac.webkit.org/changeset/166671

[Mario Sanchez Prada]

(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 :)

[James Craig]

*** Bug 132662 has been marked as a duplicate of this bug. ***