Bug 32248 – [Chromium]: Issue with setting selection

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Finnur Thorarinsson]

Chromium has some issues where the Find in page is not always starting from the right point after you close the Find session. 

This is described in this bug:
http://code.google.com/p/chromium/issues/detail?id=27910

Basically, a fix was put in for a nasty focus issue for Gmail where we set the selection to 0,0. This is supposed to be done for content-editable fields only, but was not restricted to content-editable fields. Now it is. We otherwise end up setting the selection to 0,0 more often than needed, which can in certain scenarios confuse the find algorithm into thinking the user messed with the selection during runs.

On a related note, I also noticed that Find isn't setting the selection when we end the Find session anymore (will follow this patch up with a browser test to catch this in the future).

[Finnur Thorarinsson]

Created attachment 44435 [details]
Fix for Chromium issue http://crbug.com/27910

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44435 [details] without any errors.

[Ojan Vafai]

Comment on attachment 44435 [details]
Fix for Chromium issue http://crbug.com/27910

> Index: WebKit/chromium/src/WebFrameImpl.cpp
> ===================================================================
> --- WebKit/chromium/src/WebFrameImpl.cpp	(revision 51798)
> +++ WebKit/chromium/src/WebFrameImpl.cpp	(working copy)
> @@ -1669,6 +1669,7 @@ void WebFrameImpl::setFindEndstateFocusA
> +        // No node related to the active match was focusable, so set the
> +        // active match as the selection (so that when you end the Find session,
> +        // you'll have the last thing you found highlighted) and make sure that
> +        // we have nothing focused (otherwise you might have text selected but
> +        // a link focused, which is weird).
> +        frame()->selection()->setSelection(m_activeMatch.get());
> +        frame()->document()->setFocusedNode(0);

Doesn't setSelection already set the focus appropriately? e.g. if you set the selection to some text inside a link, shouldn't the link be focused?

[Finnur Thorarinsson]

The intent here is to have the whole link appear with the focus ring around it (consistent with Firefox) and be able to press Enter to navigate to that url.

If I just do the selection part, and search for a partial text match within a link, then that partial match gets highlighted when I close the box, no focus ring appears and pressing Enter does nothing.

[Darin Fisher (:fishd, Google)]

Comment on attachment 44435 [details]
Fix for Chromium issue http://crbug.com/27910

Since the WebKit bug links to the Chromium bug, I think it is
sufficient to just include the WebKit bug link in the ChangeLog.


> Index: WebKit/chromium/src/WebFrameImpl.cpp
...
>          if (node && node != frame()->document()) {
>              // Found a focusable parent node. Set focus to it.
>              frame()->document()->setFocusedNode(node);
> +            return;
>          } else {

nit: now that have the return statement, please remove the else
statement so that you can reduce the indentation of the following
code.

R=me otherwise.

[Finnur Thorarinsson]

Created attachment 44443 [details]
Addressed review comments

[WebKit Commit Bot]

Comment on attachment 44443 [details]
Addressed review comments

Clearing flags on attachment: 44443

Committed r51818: <http://trac.webkit.org/changeset/51818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.