Bug 39792

Summary: [chromium] Accessibility focus change notifications should be triggered from ChromeClientImpl::focusedNodeChanged
Product: WebKit Reporter: Jay Civelli <jcivelli>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
First version of the patch
none
Applied fishd suggested changes
none
Prevent a crasher when NULL passed to ChromeClientImpl::focusedNodeChanged() none

Description Jay Civelli 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().
Comment 1 Jay Civelli 2010-05-26 15:35:25 PDT
Created attachment 57171 [details]
First version of the patch
Comment 2 Darin Fisher (:fishd, Google) 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.
Comment 3 Jay Civelli 2010-06-01 09:31:48 PDT
Created attachment 57552 [details]
Applied fishd suggested changes
Comment 4 Jay Civelli 2010-06-01 10:58:06 PDT
Created attachment 57568 [details]
Prevent a crasher when NULL passed to ChromeClientImpl::focusedNodeChanged()
Comment 5 Eric Seidel (no email) 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.
Comment 6 Dimitri Glazkov (Google) 2010-06-04 10:36:41 PDT
Comment on attachment 57568 [details]
Prevent a crasher when NULL passed to ChromeClientImpl::focusedNodeChanged()

this makes sense.
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2010-06-04 12:14:00 PDT
All reviewed patches have been landed.  Closing bug.