[Webkit-unassigned] [Bug 147211] AX: AccessibilityNodeObject::childrenChanged() generates too many AXLiveRegionChanged notifications

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=147211

chris fleizach <cfleizach at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #257322|review?                     |review-
              Flags|                            |

--- Comment #4 from chris fleizach <cfleizach at apple.com> ---
Comment on attachment 257322
  --> https://bugs.webkit.org/attachment.cgi?id=257322
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257322&action=review

> Source/WebCore/ChangeLog:9
> +        AccessibilityNodeObject::childrenChanged() being called many times in a short period of time may cause

change to 

AccessibilityNodeObject::childrenChanged() can be called repeatedly, generating a live region change notification each time. 
Sometimes, so many happen that VoiceOver hangs. We can use  a timer to make sure that we coalesce these notifications.
 12

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:106
> +    , liveRegionNotificationPostTimer(*this, &AccessibilityNodeObject::liveRegionChangedNotificationPostTimerFired)

i think this think should be on AXObjectCache. Right now this timer is being created for every AXObject, which also means that if you stop one then another object
will just as easily fire its own object. we want to have one instance of this timer per axOBjectCache

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:162
> +            if (liveRegionNotificationPostTimer.isActive())

if the timer is active, we can probably do the opposite and just skip this code, since we now the active timer will now cover this case too?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:166
> +            liveRegionNotificationPostTimer.startOneShot(AccessibilityLiveRegionChangeNotificationInterval);

will this work just as well if we just use 0 as the time? i have a feeling that a lot of these are being generated within the same call stack so a 0 second timer may work just as well

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:183
> +    for (AccessibilityObject* object : liveRegionObjectsSet)

this can be auto& object :  liverRegion..

> Source/WebCore/accessibility/AccessibilityNodeObject.h:210
> +    Timer liveRegionNotificationPostTimer;

internal ivars should all start with m_*

> Source/WebCore/accessibility/AccessibilityNodeObject.h:211
> +    HashSet<AccessibilityObject*> liveRegionObjectsSet;

this should be HashSet<RefPtr<AccessibilityObject>>

> LayoutTests/platform/mac/accessibility/aria-liveregions-notifications.html:48
> +        setTimeout("operation4()", 40);

if we make the timeout to be 0 seconds this may not be necessary

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:21
> +    description("This tests that ARIA live regions are sending out the correct notifications. We perform two operations to each aria region, at the end it should trigger 2 live region notifications");

2 -> two

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:31
> +            if (liveRegionChangeCount == 2) {

this doesn't necessarily catch if we only have two notifications.
we might have a 100 being fired but after 2 we cleanup.

i think in this case we should have a timeout to finish the test AFTER we get two notifications. It should be short obviously but
that will allow us to check if more notifications have come in

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:34
> +               window.testRunner.notifyDone();

use finishJSTest() instead of notifyDone

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:40
> +        window.testRunner.waitUntilDone();

use window.jsTestIsAsync = true instead of this

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:42
> +        document.getElementById("liveregion1").focus();

i don't like using the method of focus() then getting focusedElement.
instead use "accessibilityController.accessibleElementById('liveRegion1')

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:53
> +        operation1();

these should have meaningful names instead of operation*

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150723/7701cedb/attachment-0001.html>