Bug 32248

Summary: [Chromium]: Issue with setting selection
Product: WebKit Reporter: Finnur Thorarinsson <finnur.webkit>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Fix for Chromium issue http://crbug.com/27910
fishd: review-
Addressed review comments none

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:
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).
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
code.

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.