Bug 39792 - [chromium] Accessibility focus change notifications should be triggered from ChromeClientImpl::focusedNodeChanged
Summary: [chromium] Accessibility focus change notifications should be triggered from ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-26 15:32 PDT by Jay Civelli
Modified: 2010-06-04 12:14 PDT (History)
1 user (show)

See Also:


Attachments
First version of the patch (3.53 KB, patch)
2010-05-26 15:35 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Applied fishd suggested changes (3.53 KB, patch)
2010-06-01 09:31 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Prevent a crasher when NULL passed to ChromeClientImpl::focusedNodeChanged() (3.72 KB, patch)
2010-06-01 10:58 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.