Bug 33923 – REGRESSION (Safari 4): AXValueChanged no longer sent for text area scrollbars

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Beth Dakin]

<rdar://problem/6942686>

When we switched off of AppKit form controls, we stopped sending AXValueChanged notifications when textareas and other scrollable regions inside the WebView are scrolled.

[Beth Dakin]

Created attachment 47075 [details]
Patch

[Oliver Hunt]

Comment on attachment 47075 [details]
Patch

r=me

[Darin Adler]

Comment on attachment 47075 [details]
Patch

I know Oliver already reviewed by I have a few comments and questions.

> +void AXObjectCache::postNotification(AccessibilityObject* obj, AXNotification notification, bool postToElement, PostType postType)

New code should use "object", not "obj". I understand this was just copied and pasted, but still I would prefer a word to an abbreviation.

>      void postNotification(RenderObject*, AXNotification, bool postToElement, PostType = PostAsynchronously);
> +    void postNotification(AccessibilityObject*, AXNotification, bool postToElement, PostType = PostAsynchronously);

I think the new postNotification code should share code with the old one. Maybe the old one could call the new one?

> +    return adoptRef(new AccessibilityScrollbar());

No parentheses needed here.

> +        return 0.0f;

Just 0 is our style, not 0.0f.

> +class AccessibilityScrollbar : public AccessibilityObject {
> +    
> +public:

No blank line needed.

> +    virtual ~AccessibilityScrollbar() { }

No need to define this destructor explicitly. It should work fine without this.

> +    virtual AccessibilityRole roleValue() const { return ScrollBarRole; }

Can’t you call setRoleValue instead of overriding this?

> +    virtual bool accessibilityIsIgnored() const { return false; }

I think this should be private since it would be a programming mistake to call it directly.

> +    virtual IntSize size() const { return IntSize(); }
> +    virtual IntRect elementRect() const { return IntRect(); }
> +    virtual AccessibilityObject* parentObject() const { return 0; }
> +    virtual float valueForRange() const;

These could probably also all be private.

Is it really right for these all to be empty and zero in this way? Shouldn’t the scrollbar be a child of the accessibility object representing the container it scrolls or something?

[Beth Dakin]

I will update my patch to fix all of the things you mentioned, but I want to respond to you last comment in the meantime.

> 
> Is it really right for these all to be empty and zero in this way? Shouldn’t
> the scrollbar be a child of the accessibility object representing the container
> it scrolls or something?

There are three options for the empty-and-zero functions thing:

1. They are empty and zero.
2. I make it so those functions are no longer pure virtual, and therefore, they have no implementation in AccessibilityScrollbar.
3. They return real values. 

#3 may seem obviously right to you, but it was not to me. To my knowledge, scrollbars in accessibility only need to send out this notification and potentially report on their placement (hence the implementation of valueForRange() in the class), so they don't need to report on their size or anything. I arbitrarily selected between the three options because there was no clear winner to me, but I am open to doing it a different way.

Since, as I explained above, to my knowledge scrollbars do not need to do anything else in accessibility besides send this notification, I did not make them a child of the container it scrolls. I considered doing it that way, but a renderer for the container is not available in Scrollbar::scroll(), so it didn't make sense to me to find a spot where the renderer is available since I don't know of any value of having the scrollbar be a child of the container it scrolls.

[Beth Dakin]

Darin, I am adding you as a cc since I would like your feedback on my comment to your comment.

[Darin Adler]

I think you should add ASSERT_NOT_REACHED as we discussed.

[Beth Dakin]

With Darin's feedback, I committed r53644.

[Eric Seidel (no email)]

Looks like this broke Leopard:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53644%20(9697)/platform/mac/editing/deleting/backward-delete-pretty-diff.html

[Beth Dakin]

Hmm, I am not convinced that this change could have caused such a failure. Are you sure it was this change?

[Eric Seidel (no email)]

I was just looking at http://build.webkit.org/console.

just before this checkin, r53643 is green: http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Tests%29/builds/9696

this checkin, r53644 is RED:
http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Tests%29/builds/9697

the next checkin 53645 is RED:
http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Tests%29/builds/9698
(for the same reason)

It's possible something changed on the bot.  But the buildbot seems to incriminate r53644 which was this bug.
http://trac.webkit.org/changeset/53644

[Eric Seidel (no email)]

The AX notification stuff is suspect, see bug 33300.

[Eric Seidel (no email)]

All the tests pass when I do:
run-webkit-tests --release -i platform/mac/accessibility/change-notification-on-scroll.html

This checkin is definitely at fault.  Not sure why yet.  We should roll it out if we don't have a fix in the works or plan to skip this new test, no need to leave the bots green.

[Eric Seidel (no email)]

CCing author of http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/editing/deleting/backward-delete.html

and cfleizach (who wrote the DRT AX system).

[Eric Seidel (no email)]

I ran
run-webkit-tests --release --guard
platform/mac/accessibility/change-notification-on-scroll.html

and saw no failures.

[Beth Dakin]

I will add the test to the Skipped list for now.

[Eric Seidel (no email)]

All tests just passed on the bot?
http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Tests%29/builds/9710

Maybe this change caused a flakey test?

[Eric Seidel (no email)]

Attachment 47075 [details] was posted by a committer and has review+, assigning to Beth Dakin for commit.

[Eric Seidel (no email)]

Comment on attachment 47075 [details]
Patch

Obsoleting this patch since it was landed.

[James Craig]

Closing.