Bug 30832 - Notify the chrome when the focused node has changed.
Summary: Notify the chrome when the focused node has changed.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-27 13:06 PDT by Evan Stade
Modified: 2009-10-30 14:01 PDT (History)
4 users (show)

See Also:


Attachments
proposed patch (3.96 KB, patch)
2009-10-28 17:54 PDT, Evan Stade
eric: review-
Details | Formatted Diff | Diff
style fix (3.96 KB, patch)
2009-10-28 18:20 PDT, Evan Stade
levin: review-
Details | Formatted Diff | Diff
more fixes (3.88 KB, patch)
2009-10-29 11:47 PDT, Evan Stade
levin: review-
Details | Formatted Diff | Diff
stubs (15.77 KB, patch)
2009-10-30 11:58 PDT, Evan Stade
darin: review+
Details | Formatted Diff | Diff
changelog desc changed (14.75 KB, patch)
2009-10-30 12:13 PDT, Evan Stade
levin: review+
levin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Stade 2009-10-27 13:06:32 PDT
This is similar to AX code that is already in place, except that this also informs the chrome when there stops being a focused node. This is needed for Chromium to show the anchor for links that have keyboard focus.

chromium side is here: http://codereview.chromium.org/337032

If there is some sort of test I should add for this, I'll be glad to.
Comment 1 Evan Stade 2009-10-28 17:54:46 PDT
Created attachment 42073 [details]
proposed patch
Comment 2 Eric Seidel (no email) 2009-10-28 17:58:30 PDT
Comment on attachment 42073 [details]
proposed patch

You'll need to mark patches r=? for them to be reviewed.
See: http://webkit.org/coding/contributing.html

It's not clear to me why this is not already covered by existing AX apis.

This style is incorrect:
+    if (!focusChangeBlocked) {
+        page()->chrome()->focusedNodeChanged(m_focusedNode.get());
+    }
no {} around one-line ifs.

Please explain why no other port needs this/why this can't be implemented on top of existing AX infrastructure in WebCore/WebKit?
Comment 3 Eric Seidel (no email) 2009-10-28 18:03:03 PDT
Comment on attachment 42073 [details]
proposed patch

r- for the above mentioned reasons.  Thanks for marking it r? though. :)
Comment 4 Evan Stade 2009-10-28 18:05:01 PDT
other ports could use it if they wanted to, I suppose. I wasn't sure they'd want to, since it is similar to the AX code, and I didn't want to change the way AX worked because of the possibility of ill consequences for those other platforms.

The difference from the AX infrastructure is that AX doesn't tell us when a node loses focus. (notice difference in the conditional)
Comment 5 Evan Stade 2009-10-28 18:20:55 PDT
Created attachment 42075 [details]
style fix

also, I don't view this as an accessibility feature any more than supporting tab to navigate a page is an accessibility feature.
Comment 6 Darin Adler 2009-10-28 20:22:49 PDT
Comment on attachment 42075 [details]
style fix

A general purpose feature like this should not be added inside PLATFORM(CHROMIUM) ifdefs.
Comment 7 David Levin 2009-10-28 21:21:46 PDT
Please add a bug link to the changelog as well (see other changelog entries).
Comment 8 David Levin 2009-10-28 21:23:39 PDT
Comment on attachment 42075 [details]
style fix

r- for the two comments above (Darin Adler's and mine).
Comment 9 Evan Stade 2009-10-29 11:47:30 PDT
Created attachment 42121 [details]
more fixes

removed PLATFORM defines, added bu number to changelog
Comment 10 David Levin 2009-10-29 22:20:03 PDT
Comment on attachment 42121 [details]
more fixes

My only concern is that this will break the builds for non-chromium clients.

I think the easiest way to handle this is to not make this function pure  
   "virtual void focusedNodeChanged(Node*) = 0" 
but I would recommend against this.

The very nice thing about pure methods is that they help ports to quick realize when they got broken by a parameter being added/changed/etc.

So instead it would be *much better* to find each class that derives from ChromeClient and given them a default implementation that does nothing.

With this last change, I think this will be ready to go in.
Comment 11 Evan Stade 2009-10-30 11:02:52 PDT
That the reason I initially ifdefed it to chromium.

Anyway, I think I covered all the ChromeClient implementations that are in the WebKit tree (correct me if I'm wrong). There are only two, EmptyChromeClient and SVGChromeClient (which inherits from the former). All the others are presumably in other repositories (for example, chrome's is in the chromium tree).

So whoever rolls this change into the chromium tree will need to simultaneously land my chrome-side change (actually I think it would work to land the chrome side change first). I don't know what the procedure is for other projects. Presumably this will break them and they will promptly realize they need a stub implementation.
Comment 12 David Levin 2009-10-30 11:25:20 PDT
(In reply to comment #11)
> Anyway, I think I covered all the ChromeClient implementations that are in the
> WebKit tree (correct me if I'm wrong).

git grep "public WebCore::ChromeClient"
   WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:    class ChromeClient : public WebCore::ChromeClient { 
   WebKit/mac/WebCoreSupport/WebChromeClient.h:class WebChromeClient : public WebCore::ChromeClient {
   WebKit/win/WebCoreSupport/WebChromeClient.h:class WebChromeClient : public WebCore::ChromeClient {

git grep "public ChromeClient"
   WebCore/loader/EmptyClients.h:class EmptyChromeClient : public ChromeClient {
   WebCore/page/chromium/ChromeClientChromium.h:    class ChromeClientChromium : public ChromeClient {
   WebKit/haiku/WebCoreSupport/ChromeClientHaiku.h:    class ChromeClientHaiku : public ChromeClient {
   WebKit/qt/WebCoreSupport/ChromeClientQt.h:    class ChromeClientQt : public ChromeClient
   WebKit/wx/WebKitSupport/ChromeClientWx.h:class ChromeClientWx : public ChromeClient {

It looks like there are at 8, so your patch is missing 6 of them.
Comment 13 Evan Stade 2009-10-30 11:29:05 PDT
ah, I stupidly grepped in WebCore/ only. Patch coming up.
Comment 14 Evan Stade 2009-10-30 11:58:24 PDT
Created attachment 42224 [details]
stubs

I'm not sure what the convention is for all the various ChangeLog entries. I copied the same message into all of them. Would it be better to put "add a stub implementation of a ChromeClient function"?
Comment 15 Darin Adler 2009-10-30 12:11:29 PDT
(In reply to comment #14)
> I'm not sure what the convention is for all the various ChangeLog entries. I
> copied the same message into all of them. Would it be better to put "add a stub
> implementation of a ChromeClient function"?

Yes, I think so.
Comment 16 Darin Adler 2009-10-30 12:13:02 PDT
Comment on attachment 42224 [details]
stubs

I don't think we need notImplemented() inside any of these functions. That's only there when we know we need to add code, but this is a case where an empty function is probably OK for most ports at least at first. There's not necessarily anything to implement.

Patch seems fine, r=me
Comment 17 Evan Stade 2009-10-30 12:13:41 PDT
Created attachment 42225 [details]
changelog desc changed

done.
Comment 18 David Levin 2009-10-30 12:15:28 PDT
Comment on attachment 42225 [details]
changelog desc changed

I'll land it and remove the notImplemented(); from the stub functions.
Comment 19 David Levin 2009-10-30 14:01:06 PDT
Committed as http://trac.webkit.org/changeset/50351