Bug 33923

Summary: REGRESSION (Safari 4): AXValueChanged no longer sent for text area scrollbars
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: AccessibilityAssignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, ap, bdakin, cfleizach, darin, enrica, eric, jcraig, mitz
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 33300    
Bug Blocks:    
Attachments:
Description Flags
Patch none

Description Beth Dakin 2010-01-20 15:55:59 PST
<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.
Comment 1 Beth Dakin 2010-01-20 16:10:57 PST
Created attachment 47075 [details]
Patch
Comment 2 Oliver Hunt 2010-01-20 16:23:27 PST
Comment on attachment 47075 [details]
Patch

r=me
Comment 3 Darin Adler 2010-01-20 16:29:50 PST
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?
Comment 4 Beth Dakin 2010-01-20 16:47:10 PST
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.
Comment 5 Beth Dakin 2010-01-20 16:47:51 PST
Darin, I am adding you as a cc since I would like your feedback on my comment to your comment.
Comment 6 Darin Adler 2010-01-20 16:52:51 PST
I think you should add ASSERT_NOT_REACHED as we discussed.
Comment 7 Beth Dakin 2010-01-21 13:32:38 PST
With Darin's feedback, I committed r53644.
Comment 9 Beth Dakin 2010-01-21 15:06:12 PST
Hmm, I am not convinced that this change could have caused such a failure. Are you sure it was this change?
Comment 10 Eric Seidel (no email) 2010-01-21 15:16:34 PST
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
Comment 11 Eric Seidel (no email) 2010-01-21 16:23:50 PST
The AX notification stuff is suspect, see bug 33300.
Comment 12 Eric Seidel (no email) 2010-01-21 16:26:43 PST
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.
Comment 13 Eric Seidel (no email) 2010-01-21 16:28:31 PST
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).
Comment 14 Eric Seidel (no email) 2010-01-21 16:28:58 PST
I ran
run-webkit-tests --release --guard
platform/mac/accessibility/change-notification-on-scroll.html

and saw no failures.
Comment 15 Beth Dakin 2010-01-21 16:29:45 PST
I will add the test to the Skipped list for now.
Comment 16 Eric Seidel (no email) 2010-01-21 16:34:59 PST
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?
Comment 17 Eric Seidel (no email) 2010-02-01 16:14:27 PST
Attachment 47075 [details] was posted by a committer and has review+, assigning to Beth Dakin for commit.
Comment 18 Eric Seidel (no email) 2010-02-02 14:12:33 PST
Comment on attachment 47075 [details]
Patch

Obsoleting this patch since it was landed.
Comment 19 James Craig 2013-09-30 12:56:37 PDT
Closing.