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:

Version:

528+ (Nightly build)

Hardware:

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

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	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
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.