[ruleview] Refreshing the rule view has a delay and causes a flicker

RESOLVED FIXED in Firefox 32

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 32
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 1 obsolete attachment)

Any time the rule view needs to refresh, there is a a noticeable delay with an empty rule view (at least on my system).  For instance, change the 'style' attribute from the markup view of the selected node.  The whole rule view is emptied out and rebuilt.  In this case we should hopefully only be retemplpating what is necessary.

Also, there is a delay when switching between nodes on the markup view.  Maybe there is some performance work we could do to get it to feel a little snappier.
A really easy test case to see this:

1) Paste this into the URL bar:

data:text/html,<h2 style='color:red; font-size: 2em'>highlight me</h2><script>var count = 0;setInterval(()=>document.querySelector("h2").setAttribute("data-count", count++), 1000)</script>

2) Inspect the h2 element
3) Watch the flickers
I believe this is caused by the _clearRules call in nodeChanged: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#1478.  Maybe this could be as simple as comparing the current elementStyle.rules with the new elementStyle.rules and not clearing or creating any new editors if they were the same.
(In reply to Brian Grinstead [:bgrins] from comment #2)
> I believe this is caused by the _clearRules call in nodeChanged:
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/
> styleinspector/rule-view.js#1478.  Maybe this could be as simple as
> comparing the current elementStyle.rules with the new elementStyle.rules and
> not clearing or creating any new editors if they were the same.

This would handle the special case of an attribute or some other change that doesn't affect rules.  We could also tackle this a higher level, by fixing the flicker altogether.  For instance, this would still happen when adding a new inline style rule.

I'm not exactly sure what it would take to fix that, but I suspect it would require only conditionally removing / modifying existing rule editor elements if they should be (instead of the current nuclear approach as seen in clearRules: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#1517.  Or maybe it is just a performance issue and we could stick with this simpler approach by fixing it.
Posted patch ruleview-flicker.patch (obsolete) — Splinter Review
Don't know if this will pass tests, but it does get rid of the annoying flicker.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a5b7a2bdbc14
From our discussion on IRC:

The rule-view 'nodeChanged' function name is misleading because it also gets called whenever the style-inspector wants to refresh the view, even if the node hasn't changed.
One such refresh is triggered by the markup-view's '_mutationObserver' function which when it detects an attribute change, asks the style-inspector to refresh.

Changing attributes can indeed result in applied style changes, so we need to refresh but we should investigate 2 things:

- is there a way to detect if an attribute change actually means an applied style change
- optimize the rule-view at a finer level, so that we only update the UI elements that need updating
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #5)
> From our discussion on IRC:
> 
> The rule-view 'nodeChanged' function name is misleading because it also gets
> called whenever the style-inspector wants to refresh the view, even if the
> node hasn't changed.

What do you think of just calling it 'refreshPanel'?  This is what the computed view function is named that is called in the corresponding 'refresh' function in the styleinspector.

> One such refresh is triggered by the markup-view's '_mutationObserver'
> function which when it detects an attribute change, asks the style-inspector
> to refresh.
> 
> Changing attributes can indeed result in applied style changes, so we need
> to refresh but we should investigate 2 things:
> 
> - is there a way to detect if an attribute change actually means an applied
> style change

I'd be OK with just grabbing all the rules again an any attribute change, and allow the finer level optimization of the rule view to let us not worry about this

> - optimize the rule-view at a finer level, so that we only update the UI
> elements that need updating

I agree, but let's handle that as a follow up bug, since I think this solves a pretty big UI annoyance at a low risk.  For instance, go to google.com, inspect the <body> then search for something - things start to get pretty crazy.
A better test case, since you can see selector changes that happen to the rule view as a result of the attribute changing:

data:text/html,<style type='text/css'>h2[data-count^="2"],h2[data-count^="4"],h2[data-count^="6"],h2[data-count^="8"],h2[data-count^="0"] { color: green; } h2 { color: red; }</style><h2 style='font-size: 2em'>highlight me</h2><script>var count = 0;setInterval(()=>document.querySelector("h2").setAttribute("data-count", count++), 1000)</script>
Renames function to refreshPanel.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=dbcb6ecdb434
Assignee: nobody → bgrinstead
Attachment #8434166 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8434270 - Flags: review?(pbrosset)
> > - optimize the rule-view at a finer level, so that we only update the UI
> > elements that need updating
> 
> I agree, but let's handle that as a follow up bug, since I think this solves
> a pretty big UI annoyance at a low risk.  For instance, go to google.com,
> inspect the <body> then search for something - things start to get pretty
> crazy.

Filed Bug 1020438
Comment on attachment 8434270 [details] [diff] [review]
ruleview-flicker.patch

Review of attachment 8434270 [details] [diff] [review]:
-----------------------------------------------------------------

Changes look good to me.
Attachment #8434270 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/65b0d08a49ac
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.