Focus problem when clearing selection

When doing an unselect in a page, the focused node is kept focused but
it does not get the keyboard input anymore and for text editable
nodes there is no caret.
This was happening because the focus is messed up when the selection does
not include the focused node.
This CL ensure the selection includes the focused node to work-around that
issue.


BUG=21388
TEST=Open google.com, CTRL-F and search for something not in the page.
     ESC to close the find-box. The search field on the Google page
     should have focus and typing should work.
     Now focus a link in the page, do a find for some text not in the
     page. Press ESC. Pressing enter should trigger the link.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30999

[ojan, 2009-11-03 19:18:40]

Is there any way to write a test for this?

http://codereview.chromium.org/342099/diff/2001/2002
File webkit/api/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/342099/diff/2001/2002#newcode920
Line 920: if (focusedFrame.get()) {
You don't need the .get() here. null checks on RefPtr should work just like
regular pointers.

http://codereview.chromium.org/342099/diff/2001/2002#newcode935
Line 935: element->focus();
I'm not sure if it really matters, but this would cause web pages to see a
gratuitous blur event and a focus event. It's not a big deal, but it could be
weird depending on what they do on those events.

Can you instead just call element->updateFocusAppearance(true)? I *think* it
would achieve the same effect of restoring the previous selection without
causing a blur/focus.

http://codereview.chromium.org/342099/diff/2001/2002#newcode938
Line 938: Position::PositionIsAfterAnchor);
It's weird to me to put the selection after the focused node instead of inside
it at the beginning of the node. Maybe this code does something different than I
would expect. Am I thinking about this wrong?

Position position(focusedNode, 0, Position::PositionIsOffsetInAnchor);

[jcampan, 2009-11-03 21:31:34]

Regarding testing this behavior, I started writing an interactive UI test, but
it gets complicated as it requires sending key events and waiting for them to be
acknowledged from the renderer.
Do you think this can be done as a layout test? (I am not sure I can simulate a
keyboard event without dispatching it directly to a specific node).

http://codereview.chromium.org/342099/diff/2001/2002
File webkit/api/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/342099/diff/2001/2002#newcode920
Line 920: if (focusedFrame.get()) {
On 2009/11/03 19:18:41, ojan wrote:
> You don't need the .get() here. null checks on RefPtr should work just like
> regular pointers.

Done.

http://codereview.chromium.org/342099/diff/2001/2002#newcode935
Line 935: element->focus();
On 2009/11/03 19:18:41, ojan wrote:
> I'm not sure if it really matters, but this would cause web pages to see a
> gratuitous blur event and a focus event. It's not a big deal, but it could be
> weird depending on what they do on those events.
> 
> Can you instead just call element->updateFocusAppearance(true)? I *think* it
> would achieve the same effect of restoring the previous selection without
> causing a blur/focus.

Very good point. updateFocusAppearance() does the trick.

http://codereview.chromium.org/342099/diff/2001/2002#newcode938
Line 938: Position::PositionIsAfterAnchor);
On 2009/11/03 19:18:41, ojan wrote:
> It's weird to me to put the selection after the focused node instead of inside
> it at the beginning of the node. Maybe this code does something different than
I
> would expect. Am I thinking about this wrong?
> 
> Position position(focusedNode, 0, Position::PositionIsOffsetInAnchor);

This gone is now gone.

[ojan, 2009-11-03 21:51:44]

You can test this in a layout test using eventSender.keyDown("a").

I think you could probably simulate the blurring by using two iframes. Focus one
of them, and clear the editable selection in the other one. Then focus the
original one again. That should hit this code path, right?

http://codereview.chromium.org/342099/diff/3001/3002
File webkit/api/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/342099/diff/3001/3002#newcode928
Line 928: static_cast<Element*>(focusedNode)->updateFocusAppearance(true);
Does this work for rich-text as well? Namely:

<div contentEditable>foo bar baz</div>

[jcampan, 2009-11-04 19:08:37]

I filed a bug for adding a test for making sure we don't regress
(http://crbug.com/26709)

http://codereview.chromium.org/342099/diff/3001/3002
File webkit/api/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/342099/diff/3001/3002#newcode928
Line 928: static_cast<Element*>(focusedNode)->updateFocusAppearance(true);
On 2009/11/03 21:51:44, ojan wrote:
> Does this work for rich-text as well? Namely:
> 
> <div contentEditable>foo bar baz</div>
Yes it does but it selects all the text.
I reverted to setting the selection.

[ojan, 2009-11-04 19:10:57]

LGTM