Bug 140280 – Web Inspector: REGRESSION: Clicking selected item in the sidebar second time should scroll to the corresponding line

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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.

[Radar WebKit Bug Importer]

<rdar://problem/19422366>

[Timothy Hatcher]

This should already work since out TreeOutline's have allowsRepeatSelection set to true. Maybe it isn't set to true?

[Nikita Vasilyev]

(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

[Timothy Hatcher]

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.

[Brian Burg]

> (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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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.

[Brian Burg]

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);

[Brian Burg]

(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.

[Nikita Vasilyev]

(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

[Brian Burg]

(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.

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

Created attachment 246366 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 246366 [details]
Patch

Whoops, wrong patch.

[Nikita Vasilyev]

Created attachment 246367 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 246367 [details]
Patch

Clearing flags on attachment: 246367

Committed r179973: <http://trac.webkit.org/changeset/179973>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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.