Bug 157709 - Web Inspector: Many DataGrid instances do not save/restore their scroll position
Summary: Web Inspector: Many DataGrid instances do not save/restore their scroll position
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: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-14 09:23 PDT by Timothy Hatcher
Modified: 2016-05-16 10:41 PDT (History)
7 users (show)

See Also:


Attachments
Patch (12.83 KB, patch)
2016-05-14 12:10 PDT, Timothy Hatcher
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2016-05-14 09:23:39 PDT
There are two bugs here. ContentView's are never restoring their scroll positions from BackFrowardEntry as expected because a new BackFrowardEntry is created each time. Second, many ContentViews don't implement scrollableElements.
Comment 1 Radar WebKit Bug Importer 2016-05-14 09:23:57 PDT
<rdar://problem/26286090>
Comment 2 Timothy Hatcher 2016-05-14 12:10:30 PDT
Created attachment 278941 [details]
Patch
Comment 3 BJ Burg 2016-05-16 08:43:19 PDT
Comment on attachment 278941 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:96
> +        // is undefined, so we want to show with a state last used.

Initially I was worried this could reuse scroll positions where it's not appropriate. For example, two timeline recordings are made, and switching between recordings would reuse the last scroll positions.

I'm not sure this will be a problem though, since we create new ContentViews for each representedObject instance. Or, at least, that's what we should be doing. There may be some places that still use singletons (i.e., storage views) where this may have undesirable behavior.
Comment 4 Timothy Hatcher 2016-05-16 08:46:17 PDT
Comment on attachment 278941 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:96
>> +        // is undefined, so we want to show with a state last used.
> 
> Initially I was worried this could reuse scroll positions where it's not appropriate. For example, two timeline recordings are made, and switching between recordings would reuse the last scroll positions.
> 
> I'm not sure this will be a problem though, since we create new ContentViews for each representedObject instance. Or, at least, that's what we should be doing. There may be some places that still use singletons (i.e., storage views) where this may have undesirable behavior.

Yes, we have different views in that case.
Comment 5 Timothy Hatcher 2016-05-16 10:41:34 PDT
https://trac.webkit.org/r200947