Bug 181279 - Web Inspector: Find next / previous within a resource content view does not have bouncy highlight when editor scrolls
Summary: Web Inspector: Find next / previous within a resource content view does not h...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-03 21:48 PST by Joseph Pecoraro
Modified: 2018-01-08 09:42 PST (History)
6 users (show)

See Also:

Attachments
[PATCH] Proposed Fix (4.42 KB, patch)
2018-01-03 21:55 PST, Joseph Pecoraro 		no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (5.48 KB, patch)
2018-01-04 17:54 PST, Joseph Pecoraro 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2018-01-03 21:48:46 PST 
Find next / previous within a resource content view does not have bouncy highlight when editor scrolls

Steps to Reproduce:
1. Inspect webkit.org
2. Show Debugger tab
3. Select "global.js"
4. Show Find banner
5. Search for "document"
5. Search forward a few times
  => When the ContentView scrolls to review a new result there is no bouncy highlight

Notes:
- We hide the bouncy highlight on scrolls, but the scroll to the new position hid the bouncy highlight!
- We also appear to be leaking scroll handlers if we search quickly
Comment 1 Radar WebKit Bug Importer 2018-01-03 21:50:21 PST 
<rdar://problem/36291097>
Comment 2 Joseph Pecoraro 2018-01-03 21:55:44 PST 
Created attachment 330445 [details]
[PATCH] Proposed Fix
Comment 3 Devin Rousso 2018-01-04 13:45:20 PST 
Comment on attachment 330445 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=330445&action=review

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:1113
> +            if (scrollCount < 2)

This seems very risky.  Is there a way we can add an "ignoreScroll" member variable that is true whenever we are moving to a highlight position?
Comment 4 Joseph Pecoraro 2018-01-04 15:21:58 PST 
Comment on attachment 330445 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=330445&action=review

>> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:1113
>> +            if (scrollCount < 2)
> 
> This seems very risky.  Is there a way we can add an "ignoreScroll" member variable that is true whenever we are moving to a highlight position?

I tried to explain why I didn't do that in the ChangeLog. I think that would be more code, and more error prone, then just triggering this removal after a couple scroll events. I didn't spend too much time thinking about how we could determine if the scroll was programmatic or not. The intent of this scroll handler is if the user is scrolling while the bouncy highlight is up then hide the bouncy. I think that is still achieved with the idea of firing it after 2 scroll.

The best solution would be to re-position the bouncy highlight instead of removing it. I could try to do that.
Comment 5 Joseph Pecoraro 2018-01-04 17:54:34 PST 
Created attachment 330504 [details]
[PATCH] Proposed Fix
Comment 6 BJ Burg 2018-01-08 09:21:45 PST 
Comment on attachment 330504 [details]
[PATCH] Proposed Fix

r=me
Comment 7 WebKit Commit Bot 2018-01-08 09:42:20 PST 
Comment on attachment 330504 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 330504

Committed r226517: <https://trac.webkit.org/changeset/226517>
Comment 8 WebKit Commit Bot 2018-01-08 09:42:22 PST 
All reviewed patches have been landed.  Closing bug.