Bug 179291

<rdar://problem/35352660>

This is likely caused by StyleDetailsPanel.prototype._refreshPreservingScrollPosition.

This happens because view.addSubview(childView) appends childView.element to the DOM immediately. Then it sits in the DOM empty until childView.layout is called.

Created attachment 331357 [details] Patch

Created attachment 331358 [details] [Animated GIF] With patch applied

Comment on attachment 331357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331357&action=review This doesn't feel like the right thing to do. Can you describe how the original problem manifested and why this solves it? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:96 > - this._propertiesEditor.needsLayout(); > + this._propertiesEditor.updateLayout(); This one doesn't look necessary at all, the view was just created it hasn't even done its initialLayout yet, right? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:114 > this.addSubview(section); > - section.needsLayout(); > + section.updateLayout(); > this._sections.push(section); This seems like it can cause a layout / style recalc in a loop issue.

(In reply to Joseph Pecoraro from comment #6) > Comment on attachment 331357 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331357&action=review > > This doesn't feel like the right thing to do. Can you describe how the > original problem manifested and why this solves it? As I mentioned in: (In reply to Nikita Vasilyev from comment #3) > This happens because view.addSubview(childView) appends childView.element to > the DOM immediately. Then it sits in the DOM empty until childView.layout is > called. Added a section (SpreadsheetCSSStyleDeclarationSection) becomes a two-stage process: 1. Immediately appending the section element created in SpreadsheetCSSStyleDeclarationSection's constructor. 2. Appending everything else on requestAnimationFrame. Replacing needsLayout with updateLayout makes it a single operation again. Please let me know if there's a better way of doing this. I don't know how to solve this without drastically changing the View class.

Comment on attachment 331357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331357&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:96 >> + this._propertiesEditor.updateLayout(); > > This one doesn't look necessary at all, the view was just created it hasn't even done its initialLayout yet, right? I think a way of mitigating this would be to add some logic that says "if we add a subview while we are calling initialLayout()/layout(), also call initialLayout()/layout() on that added View)". I feel like this might mitigate issues on other areas as well (although I can't think of anything off the top of my head now).

(In reply to Nikita Vasilyev from comment #7) > (In reply to Joseph Pecoraro from comment #6) > > Comment on attachment 331357 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=331357&action=review > > > > This doesn't feel like the right thing to do. Can you describe how the > > original problem manifested and why this solves it? > > As I mentioned in: > > (In reply to Nikita Vasilyev from comment #3) > > This happens because view.addSubview(childView) appends childView.element to > > the DOM immediately. Then it sits in the DOM empty until childView.layout is > > called. This should be in the ChangeLog then, not just Bugzilla comments!

(In reply to Devin Rousso from comment #8) > Comment on attachment 331357 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331357&action=review > > >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:96 > >> + this._propertiesEditor.updateLayout(); > > > > This one doesn't look necessary at all, the view was just created it hasn't even done its initialLayout yet, right? > > I think a way of mitigating this would be to add some logic that says "if we > add a subview while we are calling initialLayout()/layout(), also call > initialLayout()/layout() on that added View)". I feel like this might > mitigate issues on other areas as well (although I can't think of anything > off the top of my head now). Subviews added during the parent's layout pass (either in initialLayout or layout), will be visited during subtree layout after the parent layout finishes. I'm not sure we want to change the policy, so that children added during a layout pass are updated before returning from addSubview/insertSubview. This change would mean that a view could no longer depend on its parent being in a consistent state when it updates itself.

Based our offline discussion, I think this can be greatly simplified by: 1) Removing StyleDetailsPanel.prototype.refresh and StyleDetailsPanel.Event.Refreshed. Only GeneralStyleDetailsSidebarPanel listeners for this event, and it's event handler is redundant: - GeneralStyleDetailsSidebarPanel._filterDidChange calls filterDidChange on the panel, which isn't needed. The panel has already updated it's filter bar when it refreshed itself! - It isn't necessary to toggle "filter-in-progress" on the ContentView every time a child panel is refreshed. A refresh (layout) can happen for reasons other than the filter changing. Furthermore, the ContentView classList should already be in the correct state (either it's the first refresh and the filter doesn't exist yet, or the filter changed after the panel did its initialLayout, and StyleDetailsPanel was notified via FilterBar.Event.FilterDidChange). 2) Refactoring panels to use initialLayout/layout (they are already Views!). The `significantChange` flag can be eliminated by having subclasses perform a layout (or not) based on the value of the flag.

Created attachment 336414 [details] Patch (In reply to Matt Baker from comment #11) > Based our offline discussion, I think this can be greatly simplified by: > > 1) Removing StyleDetailsPanel.prototype.refresh and > StyleDetailsPanel.Event.Refreshed. Only GeneralStyleDetailsSidebarPanel > listeners for this event, and it's event handler is redundant: > - GeneralStyleDetailsSidebarPanel._filterDidChange calls filterDidChange > on the panel, which isn't needed. The panel has already updated it's filter > bar when it refreshed itself! > - It isn't necessary to toggle "filter-in-progress" on the ContentView > every time a child panel is refreshed. A refresh (layout) can happen for > reasons other than the filter changing. Furthermore, the ContentView > classList should already be in the correct state (either it's the first > refresh and the filter doesn't exist yet, or the filter changed after the > panel did its initialLayout, and StyleDetailsPanel was notified via > FilterBar.Event.FilterDidChange). > > 2) Refactoring panels to use initialLayout/layout (they are already Views!). > The `significantChange` flag can be eliminated by having subclasses perform > a layout (or not) based on the value of the flag. This was a lot of changes and it didn't stop there. I have a small patch for now that does not remove refresh method on all panels.

Comment on attachment 336414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336414&action=review r=me, with one minor comment. Nice work! > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:46 > + this._pendingRefresh = false; Let's call this `_shouldRefreshSubviews` to make it clear what it means and that it's a flag. The "shouldRefresh" prefix is used elsewhere too, so it's good for consistency.

Created attachment 336420 [details] Patch

Comment on attachment 336420 [details] Patch Clearing flags on attachment: 336420 Committed r229922: <https://trac.webkit.org/changeset/229922>

All reviewed patches have been landed. Closing bug.