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.
<rdar://problem/19422366>
This should already work since out TreeOutline's have allowsRepeatSelection set to true. Maybe it isn't set to true?
(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
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.
> (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.
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.
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 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);
(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.
(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 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.
(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.
Created attachment 246366 [details] Patch
Comment on attachment 246366 [details] Patch Whoops, wrong patch.
Created attachment 246367 [details] Patch
Comment on attachment 246367 [details] Patch Clearing flags on attachment: 246367 Committed r179973: <http://trac.webkit.org/changeset/179973>
All reviewed patches have been landed. Closing bug.
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.