Summary: | REGRESSION (Safari 4): AXValueChanged no longer sent for text area scrollbars | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||
Component: | Accessibility | Assignee: | 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
Beth Dakin
2010-01-20 15:55:59 PST
Created attachment 47075 [details]
Patch
Comment on attachment 47075 [details]
Patch
r=me
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? 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.
Darin, I am adding you as a cc since I would like your feedback on my comment to your comment. I think you should add ASSERT_NOT_REACHED as we discussed. 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 Hmm, I am not convinced that this change could have caused such a failure. Are you sure it was this change? 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 The AX notification stuff is suspect, see bug 33300. 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. 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). I ran run-webkit-tests --release --guard platform/mac/accessibility/change-notification-on-scroll.html and saw no failures. I will add the test to the Skipped list for now. 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? Attachment 47075 [details] was posted by a committer and has review+, assigning to Beth Dakin for commit.
Comment on attachment 47075 [details]
Patch
Obsoleting this patch since it was landed.
Closing. |