|Summary:||Add a way for Widget to know about focus loss|
|Product:||WebKit||Reporter:||Stuart Morgan <stuartmorgan>|
|Component:||WebCore Misc.||Assignee:||Stuart Morgan <stuartmorgan>|
|Severity:||Normal||CC:||amohr, commit-queue, fishd, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Stuart Morgan 2010-04-21 17:20:42 PDT
Currently, Widget has a setFocus() method to tell it that it has received focus, but no way of finding out that it has lost focus. This makes it impossible to correctly implemented focus for windowless plugins that don't have a corresponding native OS view--which Chromium on the Mac, at least, does not (and there are indications in the code that Mac Safari is, or at least once was) exploring the possibility of doing this.
Comment 1 Stuart Morgan 2010-04-21 17:30:59 PDT
Created attachment 54008 [details] Adds blur() to Widget This adds blur() to Widget, along with an empty implementation on all platforms, and causes it to be called the same way setFocus() is currently called. Also adds the plumbing to connect blur() to the appropriate plugin method in Chromium (currently a no-op, but necessary for later non-WebKit Chromium changes). No actual behavior changes should result from this patch. It will be up to platforms to start using this new method as they need it.
Comment 2 WebKit Review Bot 2010-04-21 17:35:41 PDT
Attachment 54008 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/dom/Document.cpp:2873: Declaration has space between type name and * in Widget *blurWidget [whitespace/declaration]  Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Stuart Morgan 2010-04-22 10:02:15 PDT
Created attachment 54069 [details] Adds blur() to Widget Fixes style issue (in both the new code and the code it was based on)
Comment 4 Darin Fisher (:fishd, Google) 2010-05-03 09:39:06 PDT
Comment on attachment 54069 [details] Adds blur() to Widget WebCore/platform/Widget.h:155 + virtual void setFocus(); how about just passing a 'bool focused' to setFocus?
Comment 5 Stuart Morgan 2010-05-03 09:47:46 PDT
The only reason is that it would be a much more invasive change. I would have to find and change the existing implementation of setFocus in every descendent of Widget in every port, which would touch a lot more files and actually change existing logic. The chance of a build break or regression would be much higher. Since there didn't seem to be an obvious preference for bool-argument setters in other cases (e.g., Widget has show() and hide() rather than setVisible(bool)), I opted for the simpler change. I can do it the other way if people feel strongly.
Comment 6 Darin Fisher (:fishd, Google) 2010-05-04 14:19:38 PDT
It just struck me that 'setFocus' and 'blur' do not read like opposites. Maybe the new method should be named setBlur? That reads a bit funny though.
Comment 7 Stuart Morgan 2010-05-05 08:46:28 PDT
Created attachment 55128 [details] Adds boolean argument to setFocus I can't come up with a better name, so I gave the setFocus(bool) approach a shot. The QT code unfortunately has a setFocus() call as well, but that was the only other one, so finding the right things to change wasn't as bad as I feared.
Comment 8 WebKit Commit Bot 2010-05-05 09:22:21 PDT
Comment on attachment 55128 [details] Adds boolean argument to setFocus Clearing flags on attachment: 55128 Committed r58821: <http://trac.webkit.org/changeset/58821>
Comment 9 WebKit Commit Bot 2010-05-05 09:22:27 PDT
All reviewed patches have been landed. Closing bug.