[webkit-reviews] review granted: [Bug 124381] AX: Improve ARIA live region reliability by sending notifications when live regions are created/shown and hidden/destroyed : [Attachment 228348] patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 124381: AX: Improve ARIA live region reliability by sending notifications
when live regions are created/shown and hidden/destroyed
https://bugs.webkit.org/show_bug.cgi?id=124381

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

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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