Bug 48817 - Chromium: Propagate a document value changed notification on scroll.
: Chromium: Propagate a document value changed notification on scroll.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Accessibility
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-01 19:39 PDT by Chris Guillory
Modified: 2010-11-02 19:08 PDT (History)
3 users (show)

See Also:


Attachments
Send document value changed on scrollbar value changed notification. (6.24 KB, patch)
2010-11-01 19:46 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Proposed patch. (6.21 KB, patch)
2010-11-01 19:53 PDT, Chris Guillory
cfleizach: review-
cfleizach: commit‑queue-
Details | Formatted Diff | Diff
Added isAccessibilityScrollbar to use before casting (7.09 KB, patch)
2010-11-02 11:25 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Guillory 2010-11-01 19:39:30 PDT
To support basic hit testing in Chromium we need to know the top document's scroll offsets. Non-top document scroll offsets are currently lower priority.
Comment 1 Chris Guillory 2010-11-01 19:46:47 PDT
Created attachment 72620 [details]
Send document value changed on scrollbar value changed notification.
Comment 2 WebKit Review Bot 2010-11-01 19:50:47 PDT
Attachment 72620 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/accessibility/chromium/AXObjectCacheChromium.cpp:52:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Chris Guillory 2010-11-01 19:53:08 PDT
Created attachment 72621 [details]
Proposed patch.

Fixed webkit style. Minor updates.
Comment 4 chris fleizach 2010-11-01 23:00:41 PDT
Comment on attachment 72621 [details]
Proposed patch.

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

looks ok otherwise.

> WebCore/accessibility/chromium/AXObjectCacheChromium.cpp:52
> +    if (obj->roleValue() == ScrollBarRole && notification == AXValueChanged) {

i don't think obj->roleValue() is a strong enough check for doing a static_cast. If not already, I can see something like <div role="scrollbar"> in the future.

You should add a method to AccessibilityScrollbar that says "isNativeScrollbar()" or something to that effect, that will allow you to safely cast.
Comment 5 Chris Guillory 2010-11-02 11:25:32 PDT
Created attachment 72704 [details]
Added isAccessibilityScrollbar to use before casting
Comment 6 chris fleizach 2010-11-02 11:32:59 PDT
Comment on attachment 72704 [details]
Added isAccessibilityScrollbar to use before casting

r=me
Comment 7 WebKit Commit Bot 2010-11-02 16:03:19 PDT
The commit-queue encountered the following flaky tests while processing attachment 72704 [details]:

media/event-attributes.html
fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  These tests were authored by dumi@chromium.org and eric.carlson@apple.com.  The commit-queue is continuing to process your patch.
Comment 8 WebKit Commit Bot 2010-11-02 18:20:56 PDT
Comment on attachment 72704 [details]
Added isAccessibilityScrollbar to use before casting

Clearing flags on attachment: 72704

Committed r71198: <http://trac.webkit.org/changeset/71198>
Comment 9 WebKit Commit Bot 2010-11-02 18:21:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 WebKit Commit Bot 2010-11-02 19:08:45 PDT
The commit-queue encountered the following flaky tests while processing attachment 72704 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  These tests were authored by dumi@chromium.org.  The commit-queue is continuing to process your patch.