Bug 32248 - [Chromium]: Issue with setting selection
Summary: [Chromium]: Issue with setting selection
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2009-12-07 14:42 PST by Finnur Thorarinsson
Modified: 2009-12-07 17:22 PST (History)
2 users (show)

See Also:

Fix for Chromium issue http://crbug.com/27910 (3.61 KB, patch)
2009-12-07 14:54 PST, Finnur Thorarinsson
fishd: review-
Details | Formatted Diff | Diff
Addressed review comments (4.03 KB, patch)
2009-12-07 16:17 PST, Finnur Thorarinsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Finnur Thorarinsson 2009-12-07 14:42:12 PST
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:

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).
Comment 1 Finnur Thorarinsson 2009-12-07 14:54:17 PST
Created attachment 44435 [details]
Fix for Chromium issue http://crbug.com/27910
Comment 2 WebKit Review Bot 2009-12-07 14:59:30 PST
style-queue ran check-webkit-style on attachment 44435 [details] without any errors.
Comment 3 Ojan Vafai 2009-12-07 15:04:46 PST
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?
Comment 4 Finnur Thorarinsson 2009-12-07 15:12:30 PST
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.
Comment 5 Darin Fisher (:fishd, Google) 2009-12-07 15:58:34 PST
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

R=me otherwise.
Comment 6 Finnur Thorarinsson 2009-12-07 16:17:40 PST
Created attachment 44443 [details]
Addressed review comments
Comment 7 WebKit Commit Bot 2009-12-07 17:22:32 PST
Comment on attachment 44443 [details]
Addressed review comments

Clearing flags on attachment: 44443

Committed r51818: <http://trac.webkit.org/changeset/51818>
Comment 8 WebKit Commit Bot 2009-12-07 17:22:36 PST
All reviewed patches have been landed.  Closing bug.