Bug 33117 - Add ARIA "Live Region" support
Summary: Add ARIA "Live Region" support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-02 21:51 PST by chris fleizach
Modified: 2010-01-05 00:06 PST (History)
2 users (show)

See Also:


Attachments
patch (49.32 KB, patch)
2010-01-02 21:58 PST, chris fleizach
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2010-01-02 21:51:40 PST
Would be nice if there were support for ARIA "Live Regions" in  Safari/Webkit.

This markup tells the screen reader that the content is updating and to announce anything that changes. Great for stock tickers and chat logs on the web.

* NOTES
http://www.w3.org/TR/wai-aria/#liveregions
https://developer.mozilla.org/index.php?title=En/AJAX/WAI_ARIA_Live_Regions%2F%2FAPI_Support
http://www.google.com/search?client=safari&rls=en-us&q=ARIA+live+region+example&ie=UTF-8&oe=UTF-8
Comment 1 chris fleizach 2010-01-02 21:55:29 PST
This patch does a few things, all in the name of supporting live regions.

1) Send out LiveRegionChanges when content changes within a live region
  -- content changes if a node is added, removed or the text of an element is changed

2) it reports various aria-live attributes and provides default values where appropriate

3) it adds support to DRT to test when notifications are sent and have a facility for making sure those notifications are actually sent.

4) it adds a few new methods to AccessibilityUIElement in DRT to facilitate some tests.
Comment 2 chris fleizach 2010-01-02 21:58:41 PST
Created attachment 45755 [details]
patch
Comment 3 WebKit Review Bot 2010-01-02 22:00:23 PST
style-queue ran check-webkit-style on attachment 45755 [details] without any errors.
Comment 4 Darin Adler 2010-01-04 10:11:07 PST
Comment on attachment 45755 [details]
patch

> +    AccessibilityObject* obj = getOrCreate(renderer);
> +    if (obj)
> +        obj->contentChanged(); 

We normally avoid abbreviations like "obj" and instead use words like "object".

> +    // Used when text or attributes affecting visible content are changed.
> +    void contentChanged(RenderObject*);

I think that "Used" in this comment is confusing. It doesn't say who uses this function or how they use it. I'd prefer to use the more precise verb "called" and also to be specific about what code does or should call the function in what circumstance. Of course, we still do want to be brief.

> +    AccessibilityObject* axParent = parentObject();
> +    while (axParent) {
> +        if (axParent->supportsARIALiveRegion())
> +            return true;
> +        axParent = axParent->parentObject();
> +    }

This construction usually reads better when written as a for loop.

> +    bool isChildOfARIALiveRegion() const;

I think you mean "descendant" here, not "child". Or you could use the name "isInsideARIALiveRegion".

> +        // If we find a parent that has aria live region on, send the notification and stop processing.

Should capitalize ARIA in this comment.

> +String AccessibilityRenderObject::ariaLiveRegionStatus() const
> +{
> +    const AtomicString& liveRegionStatus = getAttribute(aria_liveAttr);
> +    
> +    // These roles have implicit live region status.
> +    if (liveRegionStatus.isEmpty()) {
> +        switch (roleValue()) {
> +        case ApplicationAlertDialogRole:
> +        case ApplicationAlertRole:
> +            return String("assertive");
> +        case ApplicationLogRole:
> +        case ApplicationStatusRole:
> +            return String("polite");
> +        case ApplicationTimerRole:
> +            return String("off");
> +        default:
> +            break;
> +        }
> +    }
> +    
> +    return liveRegionStatus;
> +}

Are the explicit String() needed here? If I was writing this, I would do a few things differently:

    1) I would have the function return a const AtomicString&.
    2) I would use pre-created AtomicString instances for the hard-coded values, which would allow (1).
    3) I would include the other roles in the switch statement.
    4) I would call getAttribute after the switch statement to save work in those cases since the value is ignored.

> +String AccessibilityRenderObject::ariaLiveRegionRelevant() const
> +{
> +    String relevant = getAttribute(aria_relevantAttr);
>  
> +    // Default aria-relevant = "additions text".
> +    if (relevant.isEmpty())
> +        return String("additions text");
> +    
> +    return relevant;
> +}

I think 

> +    bool areChildrenDirty() const { return m_childrenDirty; }

The name here isn't great. We try to have boolean member functions complete a sentence "this object <xxx>". So it would be something like "hasChildrenMarkedDirty", "hasDirtyChildrenList", "hasDirtyChildren". Probably none of those is quite right. Maybe "needsChildrenRecomputed"? What does "dirty" mean exactly here?

AppKit uses "needsDisplay", for example, in NSView, which is an example of a pretty good name for something a bit similar.

> +#define NSAccessibilityLiveRegionChangedNotification @"AXLiveRegionChanged"

Will this still compile when a future version of the header arrives that includes this? Is there a cleaner way to do it?

r=me as is -- All the comments above are optional.
Comment 5 chris fleizach 2010-01-04 10:35:24 PST
> > +String AccessibilityRenderObject::ariaLiveRegionStatus() const
> > +{
> > +    const AtomicString& liveRegionStatus = getAttribute(aria_liveAttr);
> > +    
> > +    // These roles have implicit live region status.
> > +    if (liveRegionStatus.isEmpty()) {
> > +        switch (roleValue()) {
> > +        case ApplicationAlertDialogRole:
> > +        case ApplicationAlertRole:
> > +            return String("assertive");
> > +        case ApplicationLogRole:
> > +        case ApplicationStatusRole:
> > +            return String("polite");
> > +        case ApplicationTimerRole:
> > +            return String("off");
> > +        default:
> > +            break;
> > +        }
> > +    }
> > +    
> > +    return liveRegionStatus;
> > +}
> 
> Are the explicit String() needed here? If I was writing this, I would do a few
> things differently:
> 
>     1) I would have the function return a const AtomicString&.
>     2) I would use pre-created AtomicString instances for the hard-coded
> values, which would allow (1).
>     3) I would include the other roles in the switch statement.

I've gotten this feedback (3) in the past that all values should be included in the switch statement. I've pushed back against this in the past for similar reasons. That is: 

a) only these roles matter in terms of this switch statement, 
b) there are probably 40 other roles that will never have anything but the default value
c) Adding every role seems like it would bloat the written code significantly, 
d) make it harder to human parse and 
e) cause us to modify this method every time a new role is added.

>     4) I would call getAttribute after the switch statement to save work in
> those cases since the value is ignored.
> 

We only want to return those default values in the switch statement, if the value of getAttribute is empty, so we need to call getAttribute first to determine if it's empty or not.
Comment 6 chris fleizach 2010-01-04 23:46:45 PST
http://trac.webkit.org/changeset/52786
Comment 7 chris fleizach 2010-01-05 00:01:08 PST
looking into why DRT doesn't build on leo
Comment 8 chris fleizach 2010-01-05 00:02:07 PST
not sure why this doesn't work
warning: no '-accessibilityIndexOfChild:' method found

the method has been around since 10.2
Comment 9 chris fleizach 2010-01-05 00:03:55 PST
i see.. it's been around hidden since 10.2, but only exposed in 10.6
Comment 10 chris fleizach 2010-01-05 00:06:58 PST
fixed (god willing)
http://trac.webkit.org/changeset/52789