[webkit-reviews] review granted: [Bug 33117] Add ARIA "Live Region" support : [Attachment 45755] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 4 10:11:07 PST 2010


Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 33117: Add ARIA "Live Region" support
https://bugs.webkit.org/show_bug.cgi?id=33117

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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.


More information about the webkit-reviews mailing list