Bug 36923 - [Chromium]: FindInPage issue with focus remaining on element after match is found
Summary: [Chromium]: FindInPage issue with focus remaining on element after match is f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-31 19:40 PDT by Finnur Thorarinsson
Modified: 2010-04-01 09:45 PDT (History)
3 users (show)

See Also:


Attachments
Patch for Find issue 36923 (1.30 KB, patch)
2010-03-31 19:46 PDT, Finnur Thorarinsson
no flags Details | Formatted Diff | Diff
Now with style issue fixed (1.30 KB, patch)
2010-03-31 20:01 PDT, Finnur Thorarinsson
no flags Details | Formatted Diff | Diff
Updated description (1.45 KB, patch)
2010-04-01 09:13 PDT, 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 2010-03-31 19:40:50 PDT
In Chromium:

1. Go to a page that has a text input field at the top, such as http://groups.google.com/?pli=1
2. Ctrl-F
3. Type a string that will match somewhere lower on the page (so that the page scrolls down when a match is found)
4. Click on any link on the page

Notice that the page scrolls back up and sometimes the navigation doesn't occur.

This is because in WebViewImpl::SetFocus we update the focus appearance for textfields, to work around a focus bug. This call causes the textfield at the top of the page to scroll into view, which is not what we want. 

We don't actually need the textfield to have focus once we find a match so the fix is simply to clear the focus during find (if a match is found). I will submit a patch shortly.
Comment 1 Finnur Thorarinsson 2010-03-31 19:46:00 PDT
Created attachment 52243 [details]
Patch for Find issue 36923
Comment 2 WebKit Review Bot 2010-03-31 19:58:50 PDT
Attachment 52243 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebFrameImpl.cpp:1327:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Finnur Thorarinsson 2010-03-31 20:01:48 PDT
Created attachment 52245 [details]
Now with style issue fixed
Comment 4 Ojan Vafai 2010-04-01 08:16:31 PDT
Comment on attachment 52245 [details]
Now with style issue fixed

Code looks correct to me.

> Index: WebKit/chromium/ChangeLog
> ===================================================================
> --- WebKit/chromium/ChangeLog	(revision 56886)
> +++ WebKit/chromium/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> +2010-03-31  Finnur Thorarinsson  <finnur.webkit@gmail.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] FindInPage should clear the focused node when a match has
> +        been found.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=36923


Would be nice to have a description here about how this fixes the bug, i.e. that WebFrameImpl::setFocus will try to refocus editable elements if it thinks they have focus, causing the page to scroll.
Comment 5 Finnur Thorarinsson 2010-04-01 09:13:56 PDT
Created attachment 52303 [details]
Updated description
Comment 6 Dimitri Glazkov (Google) 2010-04-01 09:26:59 PDT
Comment on attachment 52303 [details]
Updated description

ok.
Comment 7 WebKit Commit Bot 2010-04-01 09:45:51 PDT
Comment on attachment 52303 [details]
Updated description

Clearing flags on attachment: 52303

Committed r56917: <http://trac.webkit.org/changeset/56917>
Comment 8 WebKit Commit Bot 2010-04-01 09:45:56 PDT
All reviewed patches have been landed.  Closing bug.