Bug 179291 – Web Inspector: Styles Redesign: flashing when switching between nodes

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Created attachment 326053 [details]
[Animated GIF] Bug

Steps:
1. Open https://webkit.org/
2. Inspect the first child node of div#hero
3. Press Arrow down key several times

Actual:
There's a visible "jump" before paining the styles sidebar.

Expected:
There should be only one paint to display styles sidebar for a different node.

Notes:
This is a bug of the new styles sidebar. The old one works fine.

[Radar WebKit Bug Importer]

<rdar://problem/35352660>

[Nikita Vasilyev]

This is likely caused by StyleDetailsPanel.prototype._refreshPreservingScrollPosition.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 331357 [details]
Patch

[Nikita Vasilyev]

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

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

(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.

[Devin Rousso]

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).

[Joseph Pecoraro]

(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!

[Matt Baker]

(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.

[Matt Baker]

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.

[Nikita Vasilyev]

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.

[Matt Baker]

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.

[Nikita Vasilyev]

Created attachment 336420 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 336420 [details]
Patch

Clearing flags on attachment: 336420

Committed r229922: <https://trac.webkit.org/changeset/229922>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.