Bug 140280

Summary: Web Inspector: REGRESSION: Clicking selected item in the sidebar second time should scroll to the corresponding line
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Minor CC: burg, commit-queue, graouts, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Animated GIF
none
Patch
burg: review+, nvasilyev: commit-queue-
Patch
nvasilyev: review-, nvasilyev: commit-queue-
Patch none

Description Nikita Vasilyev 2015-01-08 19:36:52 PST
Created attachment 244317 [details]
Animated GIF

Clicking on a search result item in the navigation sidebar should always scroll to the corresponding line in the source view, even if has been already selected.

Steps to reproduce: 
0. Open Inspector on http://apple.com.
1. Open Resources sidebar.
2. In the Search Resource Content input field type "return".
3. Click on the first result.
4. Scroll down.
5. Click on the first result again.

Expected:
Scroll to source file to the corresponding line.

Actual:
Nothing happened.

If I manually scroll after selecting an item in the sidebar, I might want to scroll back by clicking the same sidebar item again.

The same problem exists for breakpoints in the debugger sidebar.
Comment 1 Radar WebKit Bug Importer 2015-01-08 19:37:04 PST
<rdar://problem/19422366>
Comment 2 Timothy Hatcher 2015-01-08 21:14:57 PST
This should already work since out TreeOutline's have allowsRepeatSelection set to true. Maybe it isn't set to true?
Comment 3 Nikita Vasilyev 2015-01-08 23:17:44 PST
(In reply to comment #2)
> This should already work since out TreeOutline's have allowsRepeatSelection
> set to true. Maybe it isn't set to true?

Even when it’s set to true it still exits early in WebInspector.ContentViewContainer.prototype.showContentView because currentEntry doesn’t change:

    if (currentEntry && currentEntry.contentView === contentView && Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie)) {

https://trac.webkit.org/browser/trunk/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js#L151
Comment 4 Timothy Hatcher 2015-01-10 17:21:07 PST
Oh, interesting. Then this is a regression since that code was added last year.

My gut feeling says we should do:

currentEntry.contentView.restoreFromCookie(currentEntry.cookie);
return currentEntry.contentView;

Maybe even rename BackForwardEntry's _restoreFromCookie to make it public (drop the _) and call it. That also restores scroll position, but that code isn't used for CodeMirror views. It might be useful for something else.
Comment 5 Brian Burg 2015-01-13 22:06:03 PST
> (In reply to comment #2)
> > This should already work since out TreeOutline's have allowsRepeatSelection
> > set to true. Maybe it isn't set to true?
> 
> Even when it’s set to true it still exits early in
> WebInspector.ContentViewContainer.prototype.showContentView because
> currentEntry doesn’t change:
> 
>     if (currentEntry && currentEntry.contentView === contentView &&
> Object.shallowEqual(provisionalEntry.cookie, currentEntry.cookie)) {
> 
> https://trac.webkit.org/browser/trunk/Source/WebInspectorUI/UserInterface/
> Views/ContentViewContainer.js#L151

IIRC, the scroll positions were (are?) stored separately from the ContentViewCookie intentionally, so that the interaction described will not make new history entries.

I think you should alter this early exit code path so it will show the content view but not make a new history entry if the provisional and current entry match but scroll positions don't.
Comment 6 Nikita Vasilyev 2015-02-08 20:40:41 PST
Created attachment 246255 [details]
Patch

Even though I just added only one line here, since showContentView used in a bunch of places, I need to test it more.
Comment 7 Nikita Vasilyev 2015-02-09 20:57:50 PST
https://trac.webkit.org/browser/trunk/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js#L136

    showContentView: function(contentView, cookie)

I find naming the second argument "cookie" to be both confusing and misleading at the same time. It just an object, like this:

    {
      columnNumber: 0,
      lineNumber: 433
    }

Furthermore, it’s often not related to cookies at all.
https://trac.webkit.org/browser/trunk/Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js#L187

    var cookie = positionToReveal ? {lineNumber: positionToReveal.lineNumber, columnNumber: positionToReveal.columnNumber} : {};
    WebInspector.contentBrowser.showContentViewForRepresentedObject(representedObject, cookie);

It should be called "position", "sourceCodePosition", or "positionToReveal" instead.
Comment 8 Brian Burg 2015-02-09 21:11:18 PST
Comment on attachment 246255 [details]
Patch

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

Looks good, just small comments. I think you should test back/forward navigations where the same view has multiple entries but their scroll position differs. Also, you should test that scroll-dependent behavior still works. The main example I have in mind is type profiling being based on the currently visible text editor contents.

> Source/WebInspectorUI/ChangeLog:7
> +

Please explain why the change fixes the regression.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:152
> +            currentEntry.prepareToShow(false);

please do the following for boolean arguments:

const shouldCallShown = false;
currentEntry.prepareToShow(shouldCallShown);
Comment 9 Brian Burg 2015-02-09 21:14:53 PST
(In reply to comment #7)
> https://trac.webkit.org/browser/trunk/Source/WebInspectorUI/UserInterface/
> Views/ContentViewContainer.js#L136
> 
>     showContentView: function(contentView, cookie)
> 
> I find naming the second argument "cookie" to be both confusing and
> misleading at the same time. It just an object, like this:
> 
>     {
>       columnNumber: 0,
>       lineNumber: 433
>     }
> 
> Furthermore, it’s often not related to cookies at all.
> https://trac.webkit.org/browser/trunk/Source/WebInspectorUI/UserInterface/
> Views/ResourceSidebarPanel.js#L187
> 
>     var cookie = positionToReveal ? {lineNumber:
> positionToReveal.lineNumber, columnNumber: positionToReveal.columnNumber} :
> {};
>    
> WebInspector.contentBrowser.
> showContentViewForRepresentedObject(representedObject, cookie);
> 
> It should be called "position", "sourceCodePosition", or "positionToReveal"
> instead.

'cookie' here refers to the ContentViewCookie system, which is effectively a way to serialize view state for the purposes of implementing a BackForwardList and re-opening the inspector to the same content view. Every stateful content view has a saveToCookie/restoreFromCookie method.

NavigationSidebarPanel has a similar system which is explicitly called viewStateCookie. I suppose it could be renamed in a separate patch... not high on my list of things to do though.
Comment 10 Nikita Vasilyev 2015-02-09 22:40:44 PST
(In reply to comment #8)
> Comment on attachment 246255 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246255&action=review
> 
> Looks good, just small comments. I think you should test back/forward
> navigations where the same view has multiple entries but their scroll
> position differs.

I’m afraid that’s already broken: https://bugs.webkit.org/show_bug.cgi?id=141416
Comment 11 Brian Burg 2015-02-10 06:46:16 PST
(In reply to comment #10)
> (In reply to comment #8)
> > Comment on attachment 246255 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=246255&action=review
> > 
> > Looks good, just small comments. I think you should test back/forward
> > navigations where the same view has multiple entries but their scroll
> > position differs.
> 
> I’m afraid that’s already broken:
> https://bugs.webkit.org/show_bug.cgi?id=141416

In that bug, the scroll positions are the same (the content view hasn't scrolled). Seems like a different bug to me.
Comment 12 Nikita Vasilyev 2015-02-10 18:53:15 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > Comment on attachment 246255 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=246255&action=review
> > > 
> > > Looks good, just small comments. I think you should test back/forward
> > > navigations where the same view has multiple entries but their scroll
> > > position differs.

https://cloudup.com/ctt6xehLM8S
I don’t see anything out of order here.
Comment 13 Nikita Vasilyev 2015-02-10 18:54:15 PST
Created attachment 246366 [details]
Patch
Comment 14 Nikita Vasilyev 2015-02-10 18:55:56 PST
Comment on attachment 246366 [details]
Patch

Whoops, wrong patch.
Comment 15 Nikita Vasilyev 2015-02-10 18:57:48 PST
Created attachment 246367 [details]
Patch
Comment 16 WebKit Commit Bot 2015-02-11 16:46:27 PST
Comment on attachment 246367 [details]
Patch

Clearing flags on attachment: 246367

Committed r179973: <http://trac.webkit.org/changeset/179973>
Comment 17 WebKit Commit Bot 2015-02-11 16:46:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Joseph Pecoraro 2015-03-17 14:05:26 PDT
I think this caused a regression with Inspect Element not jumping to the right place in the DOM Tree. I'll file a new bug.