Bug 44472 – Add AX notification for childrenChanged

• Component: Accessibility
• Status: RESOLVED FIXED

[Chris Guillory]

The user facing accessibility tree in the chromium port is cache in the browser process and needs to be notified to update portions of the tree as they change. This change adds the AXChildrenChanged and sends it as a platform notification.

[Chris Guillory]

Created attachment 65193 [details]
Proposed patch

Work in progress.

[Chris Guillory]

Hey Chris, can you take a look over this patch? We're trying to add the ability to dynamically update the browser side cache of the accessibility tree within Chromium.

[WebKit Review Bot]

Attachment 65193 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/accessibility/chromium/AXObjectCacheChromium.cpp:68:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/accessibility/chromium/AXObjectCacheChromium.cpp:67:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 65193 [details]
Proposed patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 65849)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> [phone]  Chris Guillory   <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add AX notification for childrenChanged.
> +        https://bugs.webkit.org/show_bug.cgi?id=44472
> +
> +        * accessibility/AXObjectCache.h:
> +        (WebCore::AXObjectCache::):
> +        * accessibility/AccessibilityRenderObject.cpp:
> +        (WebCore::AccessibilityRenderObject::childrenChanged):
> +        * accessibility/chromium/AXObjectCacheChromium.cpp:
> +        (WebCore::AXObjectCache::postPlatformNotification):
> +        * page/chromium/ChromeClientChromium.h:
> +
>  2010-08-23  Darin Adler  <[e-mail]>
>  
>          Reviewed by Geoff Garen.
> Index: WebCore/accessibility/AXObjectCache.h
> ===================================================================
> --- WebCore/accessibility/AXObjectCache.h	(revision 65848)
> +++ WebCore/accessibility/AXObjectCache.h	(working copy)
> @@ -107,6 +107,7 @@ public:
>      enum AXNotification {
>          AXActiveDescendantChanged,
>          AXCheckedStateChanged,
> +        AXChildrenChanged,
>          AXFocusedUIElementChanged,
>          AXLayoutComplete,
>          AXLoadComplete,
> Index: WebCore/accessibility/AccessibilityRenderObject.cpp
> ===================================================================
> --- WebCore/accessibility/AccessibilityRenderObject.cpp	(revision 65848)
> +++ WebCore/accessibility/AccessibilityRenderObject.cpp	(working copy)
> @@ -3236,6 +3236,8 @@ void AccessibilityRenderObject::children
>      if (!m_renderer)
>          return;
>      
> +    BOOL sentChildrenChanged = false;
> +    
>      // Go up the accessibility parent chain, but only if the element already exists. This method is
>      // called during render layouts, minimal work should be done. 
>      // If AX elements are created now, they could interrogate the render tree while it's in a funky state.
> @@ -3245,6 +3247,12 @@ void AccessibilityRenderObject::children
>              continue;
>          
>          AccessibilityRenderObject* axParent = static_cast<AccessibilityRenderObject*>(parent);
> +        
> +        if (!sentChildrenChanged) {
> +            axObjectCache()->postNotification(axParent->renderer(), AXObjectCache::AXChildrenChanged, true);
> +            sentChildrenChanged = true;
> +        }
> +        
>          // Only do work if the children haven't been marked dirty. This has the effect of blocking
>          // future live region change notifications until the AX tree has been accessed again. This
>          // is a good performance win for all parties.
> Index: WebCore/accessibility/chromium/AXObjectCacheChromium.cpp
> ===================================================================
> --- WebCore/accessibility/chromium/AXObjectCacheChromium.cpp	(revision 65848)
> +++ WebCore/accessibility/chromium/AXObjectCacheChromium.cpp	(working copy)
> @@ -56,15 +56,23 @@ void AXObjectCache::attachWrapper(Access
>  
>  void AXObjectCache::postPlatformNotification(AccessibilityObject* obj, AXNotification notification)
>  {
> -    if (notification != AXCheckedStateChanged)
> +    if (notification != AXCheckedStateChanged && notification != AXChildrenChanged)
>          return;
>  
>      if (!obj || !obj->document() || !obj->documentFrameView())
>          return;
>  
>      ChromeClientChromium* client = toChromeClientChromium(obj->documentFrameView());
> -    if (client)
> -        client->didChangeAccessibilityObjectState(obj);
> +    if (client) {
> +        switch(notification) {
> +            case AXCheckedStateChanged:
> +                client->didChangeAccessibilityObjectState(obj);
> +                break;
> +            case AXChildrenChanged:
> +                client->didChangeAccessibilityObjectChildren(obj);
> +                break;
> +        }
> +    }
>  }
>  
>  void AXObjectCache::handleFocusedUIElementChanged(RenderObject*, RenderObject*)
> Index: WebCore/page/chromium/ChromeClientChromium.h
> ===================================================================
> --- WebCore/page/chromium/ChromeClientChromium.h	(revision 65848)
> +++ WebCore/page/chromium/ChromeClientChromium.h	(working copy)
> @@ -55,6 +55,9 @@ public:
>  
>      // Notifies embedder that the state of an accessibility object has changed.
>      virtual void didChangeAccessibilityObjectState(AccessibilityObject*) = 0;
> +    
> +    // Notified embedder that the children of an accessibility object has changed.
> +    virtual void didChangeAccessibilityObjectChildren(AccessibilityObject*) = 0;
>  };
>  
>  } // namespace WebCore
> Index: WebKit/chromium/ChangeLog
> ===================================================================
> --- WebKit/chromium/ChangeLog	(revision 65849)
> +++ WebKit/chromium/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> [phone]  Chris Guillory   <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add AX notification for childrenChanged.
> +        https://bugs.webkit.org/show_bug.cgi?id=44472
> +
> +        * public/WebViewClient.h:
> +        (WebKit::WebViewClient::didChangeAccessibilityObjectChildren):
> +        * src/ChromeClientImpl.cpp:
> +        (WebKit::ChromeClientImpl::didChangeAccessibilityObjectChildren):
> +        * src/ChromeClientImpl.h:
> +
>  2010-08-23  Kenneth Russell  <[e-mail]>
>  
>          Reviewed by Dimitri Glazkov.
> Index: WebKit/chromium/public/WebViewClient.h
> ===================================================================
> --- WebKit/chromium/public/WebViewClient.h	(revision 65848)
> +++ WebKit/chromium/public/WebViewClient.h	(working copy)
> @@ -277,6 +277,9 @@ public:
>  
>      // Notifies embedder that the state of an accessibility object has changed.
>      virtual void didChangeAccessibilityObjectState(const WebAccessibilityObject&) { }
> +    
> +    // Notifies embedder that the children of an accessibility object has changed.
> +    virtual void didChangeAccessibilityObjectChildren(const WebAccessibilityObject&) { }
>  
>  
>      // Developer tools -----------------------------------------------------
> Index: WebKit/chromium/src/ChromeClientImpl.cpp
> ===================================================================
> --- WebKit/chromium/src/ChromeClientImpl.cpp	(revision 65848)
> +++ WebKit/chromium/src/ChromeClientImpl.cpp	(working copy)
> @@ -712,6 +712,13 @@ void ChromeClientImpl::didChangeAccessib
>          m_webView->client()->didChangeAccessibilityObjectState(WebAccessibilityObject(obj));
>  }
>  
> +void ChromeClientImpl::didChangeAccessibilityObjectChildren(WebCore::AccessibilityObject* obj)
> +{
> +    // Alert assistive technology about the accessibility object children change
> +    if (obj)
> +        m_webView->client()->didChangeAccessibilityObjectChildren(WebAccessibilityObject(obj));
> +}
> +
>  #if ENABLE(NOTIFICATIONS)
>  NotificationPresenter* ChromeClientImpl::notificationPresenter() const
>  {
> Index: WebKit/chromium/src/ChromeClientImpl.h
> ===================================================================
> --- WebKit/chromium/src/ChromeClientImpl.h	(revision 65848)
> +++ WebKit/chromium/src/ChromeClientImpl.h	(working copy)
> @@ -167,6 +167,7 @@ public:
>                               bool handleExternally);
>      virtual void popupClosed(WebCore::PopupContainer* popupContainer);
>      virtual void didChangeAccessibilityObjectState(WebCore::AccessibilityObject*);
> +    virtual void didChangeAccessibilityObjectChildren(WebCore::AccessibilityObject*);
>  
>      // ChromeClientImpl:
>      void setCursor(const WebCursorInfo& cursor);

WebCore/accessibility/AccessibilityRenderObject.cpp:3240
 +      
bool should be lowercase

Can you write a layout test to make sure that you're getting this notification on chrome?

[Early Warning System Bot]

Attachment 65193 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3705591

[Chris Guillory]

Created attachment 65350 [details]
Patch

I'm not sure how to write such a layout test since the update logic takes place on the browser side. Only the propagation of the new notification occurs within the renderer. Any suggestions or is there another testing mechanism within webkit that I can use?

[WebKit Review Bot]

Attachment 65350 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/accessibility/chromium/AXObjectCacheChromium.cpp:68:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/accessibility/chromium/AXObjectCacheChromium.cpp:67:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Guillory]

Created attachment 65353 [details]
Patch

Fixing case statement style errors.

[chris fleizach]

(In reply to comment #6)
> Created an attachment (id=65350) [details]
> Patch
> 
> I'm not sure how to write such a layout test since the update logic takes place on the browser side. Only the propagation of the new notification occurs within the renderer. Any suggestions or is there another testing mechanism within webkit that I can use?

I think you want to write a layout test that will make sure that the notification is fired in the right circumstances. In the Mac wrapper in DumpRenderTree, there are some mechanisms for observing notifications within layout tests. see if those make sense and if chromium's DRT wrappers can be modified to do a similar thing.

[chris fleizach]

Comment on attachment 65353 [details]
Patch

the code looks fine here, but we need a layout test, so i'm going to move to r-.
I don't know much about chromium layout tests, eric seidel would probably be a good person to ask about an approach to do this.

[Chris Guillory]

Created attachment 65789 [details]
Proposed patch

I've added a layout test that verifies the notification is received. It passes in the Chromium port of DRT (but not the Chromium testshell which is in the process of going away).

[Chris Guillory]

Created attachment 65793 [details]
Proposed patch

Fixing some typos and spacing issues in the patch.

[chris fleizach]

Comment on attachment 65789 [details]
Proposed patch

WebCore/accessibility/chromium/AXObjectCacheChromium.cpp:61
 +  
since you have a switch statement now (down below), i don't think you need this if at the top. other notifications will just fall through

WebKitTools/DumpRenderTree/chromium/AccessibilityController.h:48
 +      // Shadow to include accessibility initialization.
unneeded white space?

[chris fleizach]

Comment on attachment 65793 [details]
Proposed patch


> Index: WebCore/accessibility/chromium/AXObjectCacheChromium.cpp
> ===================================================================
> --- WebCore/accessibility/chromium/AXObjectCacheChromium.cpp	(revision 66033)
> +++ WebCore/accessibility/chromium/AXObjectCacheChromium.cpp	(working copy)
> @@ -56,15 +56,23 @@ void AXObjectCache::attachWrapper(Access
>  
>  void AXObjectCache::postPlatformNotification(AccessibilityObject* obj, AXNotification notification)
>  {
> -    if (notification != AXCheckedStateChanged)
> +    if (notification != AXCheckedStateChanged && notification != AXChildrenChanged)
>          return;
>  

I don't think you need this check, since you have a switch statement below

[Chris Guillory]

Created attachment 65807 [details]
Proposed patch

Yes, that check is unneeded for the logic. It was kind of an early exit of the function for unused notifications. I've removed it.

[chris fleizach]

Comment on attachment 65807 [details]
Proposed patch

r=me

[WebKit Commit Bot]

Comment on attachment 65807 [details]
Proposed patch

Clearing flags on attachment: 65807

Committed r66305: <http://trac.webkit.org/changeset/66305>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66305 might have broken Chromium Mac Release

[Daniel Bates]

Comment on attachment 65807 [details]
Proposed patch

>  void AXObjectCache::postPlatformNotification(AccessibilityObject* obj, AXNotification notification)
>  {
> -    if (notification != AXCheckedStateChanged)
> -        return;
> -
>      if (!obj || !obj->document() || !obj->documentFrameView())
>          return;
>  
>      ChromeClientChromium* client = toChromeClientChromium(obj->documentFrameView());
> -    if (client)
> -        client->didChangeAccessibilityObjectState(obj);
> +    if (client) {
> +        switch (notification) {
> +        case AXCheckedStateChanged:
> +            client->didChangeAccessibilityObjectState(obj);
> +            break;
> +        case AXChildrenChanged:
> +            client->didChangeAccessibilityObjectChildren(obj);
> +            break;
> +        }
> +    }

This switch statement caused compile-time failures on the Chromium Mac Release bot because it neither has a default case nor handles all the enum values of AXNotification.

[Daniel Bates]

Committed build fix in changeset 66317 <http://trac.webkit.org/changeset/66317>.

[Chris Guillory]

It's interested that the compilation was succeeded when all those other cases weren't handled in the switch statement prior to this change.

[Daniel Bates]

(In reply to comment #22)
> It's interested that the compilation was succeeded when all those other cases weren't handled in the switch statement prior to this change.

The Chromium Mac bots seems to be a bit more stringent (which in my opinion is good). They compile with the GCC flags -Wall and -Werror, which enables warnings about questionable constructions and treats all warnings as errors, respectively.

I would suggest that we enable such warnings and treat all warnings as errors on the other Chromium bots to help catch potential program errors at compile-time.