Brian Grinstead [:bgrins] Assignee
	Description • 11 years ago

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.

	Brian Grinstead [:bgrins] Assignee
	Comment 1 • 10 years ago

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

	Brian Grinstead [:bgrins] Assignee
	Comment 2 • 10 years ago

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.

	Brian Grinstead [:bgrins] Assignee
	Comment 3 • 10 years ago

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

	Brian Grinstead [:bgrins] Assignee
	Comment 4 • 10 years ago

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

	Patrick Brosset <:pbro>
	Comment 5 • 10 years ago

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

	Brian Grinstead [:bgrins] Assignee
	Comment 6 • 10 years ago

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

	Brian Grinstead [:bgrins] Assignee
	Comment 7 • 10 years ago

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>

	Brian Grinstead [:bgrins] Assignee
	Comment 8 • 10 years ago

Renames function to refreshPanel.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=dbcb6ecdb434

	Brian Grinstead [:bgrins] Assignee
	Comment 9 • 10 years ago

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

	Patrick Brosset <:pbro>
	Comment 10 • 10 years ago

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

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

Changes look good to me.

	Brian Grinstead [:bgrins] Assignee
	Comment 11 • 10 years ago

 https://hg.mozilla.org/integration/fx-team/rev/65b0d08a49ac

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/65b0d08a49ac