RESOLVED FIXED 39792
[chromium] Accessibility focus change notifications should be triggered from ChromeClientImpl::focusedNodeChanged 
https://bugs.webkit.org/show_bug.cgi?id=39792
Summary [chromium] Accessibility focus change notifications should be triggered from ...
Jay Civelli
Reported 2010-05-26 15:32:52 PDT
The accessibility notifications that a new accessible node was focused in the page are fired from the ChromeClientImpl::focus() method. This method is currently wrongly invoked every time the focus change in the page, it should only be invoked when the containing Widget gets the focus. Therefore the accessibility notifications should be fired from the more appropriate ChromeClientImpl::focusedNodeChanged().
Attachments
First version of the patch (3.53 KB, patch)
2010-05-26 15:35 PDT, Jay Civelli 		no flags
DetailsFormatted DiffDiff
Applied fishd suggested changes (3.53 KB, patch)
2010-06-01 09:31 PDT, Jay Civelli 		no flags
DetailsFormatted DiffDiff
Prevent a crasher when NULL passed to ChromeClientImpl::focusedNodeChanged() (3.72 KB, patch)
2010-06-01 10:58 PDT, Jay Civelli 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Jay Civelli
Comment 1 2010-05-26 15:35:25 PDT
Created attachment 57171 [details] First version of the patch
Darin Fisher (:fishd, Google)
Comment 2 2010-05-28 10:15:51 PDT
Comment on attachment 57171 [details] First version of the patch WebKit/chromium/src/ChromeClientImpl.cpp:195 + WebURL focusUrl; nit: webkit style is focusURL (see the Names section at http://webkit.org/coding/coding-style.html). WebKit/chromium/src/ChromeClientImpl.cpp:201 + hitTest.setURLElement(reinterpret_cast<Element*>(node)); nit: this should use static_cast since an Element "is a" Node. WebKit/chromium/src/ChromeClientImpl.cpp:211 + ASSERT_NOT_REACHED(); nit: indent by 4 spaces otherwise, LGTM. please correct those nits, and then good to commit.
Jay Civelli
Comment 3 2010-06-01 09:31:48 PDT
Created attachment 57552 [details] Applied fishd suggested changes
Jay Civelli
Comment 4 2010-06-01 10:58:06 PDT
Created attachment 57568 [details] Prevent a crasher when NULL passed to ChromeClientImpl::focusedNodeChanged()
Eric Seidel (no email)
Comment 5 2010-06-02 02:25:47 PDT
Comment on attachment 57171 [details] First version of the patch Cleared Darin Fisher's review+ from obsolete attachment 57171 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Dimitri Glazkov (Google)
Comment 6 2010-06-04 10:36:41 PDT
Comment on attachment 57568 [details] Prevent a crasher when NULL passed to ChromeClientImpl::focusedNodeChanged() this makes sense.
WebKit Commit Bot
Comment 7 2010-06-04 12:13:55 PDT
Comment on attachment 57568 [details] Prevent a crasher when NULL passed to ChromeClientImpl::focusedNodeChanged() Clearing flags on attachment: 57568 Committed r60695: <http://trac.webkit.org/changeset/60695>
WebKit Commit Bot
Comment 8 2010-06-04 12:14:00 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.