Bug 158218 - Web Inspector: scrolled Snapshot list is reset to top and drawn blank after switching back from Snapshot Comparison view
Summary: Web Inspector: scrolled Snapshot list is reset to top and drawn blank after s...
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: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-30 16:57 PDT by BJ Burg
Modified: 2016-07-07 14:27 PDT (History)
8 users (show)

See Also:


Attachments
[Video] Can't reproduce (3.53 MB, video/mp4)
2016-06-20 12:14 PDT, Nikita Vasilyev
no flags Details
[IMAGE] Issue - Blank Section (133.73 KB, image/png)
2016-06-20 13:16 PDT, Joseph Pecoraro
no flags Details
WIP (1.50 KB, patch)
2016-06-20 14:44 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (6.93 KB, patch)
2016-07-07 13:53 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2016-05-30 16:57:25 PDT
STEPS TO REPRODUCE:

1. Capture enough snapshots so that the Snapshot List table has scroll overflow.
2. Scroll down to the bottom.
3. Select two snapshots with "Compare Snapshot" button
4. Re-select "Snapshot List" path component in the secondary navigation bar.

EXPECTED:

Scroll position within the data grid is restored

ACTUAL:

Scroll position is reset, and data grid appears empty until scrolling forces relayout/repaint

NOTES:

It looks like the data grid scroll position is reset to top, but the styles of the rows are not re-synced to that position.
I think it's fine to reset the scroll position since we don't integrate secondary view changes into our back-forward list. But, the view shouldn't be blank!
Comment 1 Radar WebKit Bug Importer 2016-05-30 16:57:53 PDT
<rdar://problem/26545000>
Comment 2 BJ Burg 2016-05-30 17:03:38 PDT
This also seems to happen when switching between Allocations timeline and other timelines if the Snapshot List content view has enough snapshots to cause scroll overflow.
Comment 3 Timothy Hatcher 2016-05-31 13:46:45 PDT
We need to make sure scrollableElements is implemented for these views. This is fallout from my change to cache more things in DataGrid. The DataGrid assumes the scroll position does not change between hide and show to avoid expensive forced layouts.

I thought I got all the Timeline views, but I might have missed these views.
Comment 4 Nikita Vasilyev 2016-06-20 12:14:56 PDT
Created attachment 281659 [details]
[Video] Can't reproduce

I don't see any blank DataGrids. Let me know if I'm missing something.
Comment 5 Joseph Pecoraro 2016-06-20 13:15:41 PDT
I was able to reproduce pretty quickly in r202232. Two issues, one that caused missing records (drawn blank) and one just basic not saving the scroll position:

Steps to Reproduce:
1. Inspect this page
2. Enable the JavaScript Allocations Timeline
3. Start recording
4. Create lots of snapshots (~50+)
5. Scroll all the way down the snapshot list
6. Switch Tab away and switch tabs back
  => List is suddenly scrolled to top
7. Scroll all the way down the snapshot list
8. Compare the last two snapshots
9. Go back to snapshot list
  => List is suddenly scrolled to top, missing records
Comment 6 Joseph Pecoraro 2016-06-20 13:16:05 PDT
Created attachment 281670 [details]
[IMAGE] Issue - Blank Section
Comment 7 Nikita Vasilyev 2016-06-20 14:11:46 PDT
(In reply to comment #5)
> I was able to reproduce pretty quickly in r202232. Two issues, one that
> caused missing records (drawn blank) and one just basic not saving the
> scroll position:
> 
> Steps to Reproduce:
> 1. Inspect this page
> 2. Enable the JavaScript Allocations Timeline
> 3. Start recording
> 4. Create lots of snapshots (~50+)
> 5. Scroll all the way down the snapshot list
> 6. Switch Tab away and switch tabs back
>   => List is suddenly scrolled to top
> 7. Scroll all the way down the snapshot list
> 8. Compare the last two snapshots
> 9. Go back to snapshot list
>   => List is suddenly scrolled to top, missing records

I just reproduced the bug on this page. I'm not sure what was different before.
Comment 8 Nikita Vasilyev 2016-06-20 14:44:19 PDT
Created attachment 281679 [details]
WIP

(In reply to comment #3)
> We need to make sure scrollableElements is implemented for these views. This
> is fallout from my change to cache more things in DataGrid. The DataGrid
> assumes the scroll position does not change between hide and show to avoid
> expensive forced layouts.
> 
> I thought I got all the Timeline views, but I might have missed these views.

I assume you were talking about this change:
https://bugs.webkit.org/show_bug.cgi?id=157709

I added scrollableElements to WebInspector.HeapAllocationsTimelineView
but it didn't fix the bug.
Comment 9 Timothy Hatcher 2016-07-07 13:13:08 PDT
I am unable to reproduce this. I do see the scroll position reset to the top, but the view is not blank.

I'll look into this some more.
Comment 10 Timothy Hatcher 2016-07-07 13:37:58 PDT
I found the issue. Showing the snapshot lists does view swapping, not a full show/hide of a ContentView. So scrollableElements is not used in all cases.
Comment 11 Timothy Hatcher 2016-07-07 13:53:36 PDT
Created attachment 283045 [details]
Patch
Comment 12 BJ Burg 2016-07-07 13:57:25 PDT
Comment on attachment 283045 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:15
> +        This was duplicated in the class, removed one.

Lol
Comment 13 WebKit Commit Bot 2016-07-07 14:27:11 PDT
Comment on attachment 283045 [details]
Patch

Clearing flags on attachment: 283045

Committed r202932: <http://trac.webkit.org/changeset/202932>
Comment 14 WebKit Commit Bot 2016-07-07 14:27:15 PDT
All reviewed patches have been landed.  Closing bug.