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 Inspector | Assignee: | 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
Nikita Vasilyev
2015-01-08 19:36:52 PST
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. |