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

RESOLVED FIXED in Firefox 32

Status

RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 32
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 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.
(Assignee)

Comment 1

4 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
(Assignee)

Comment 2

4 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.
(Assignee)

Comment 3

4 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.
(Assignee)

Comment 4

4 years ago
Created attachment 8434166 [details] [diff] [review]
ruleview-flicker.patch

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
(Assignee)

Comment 6

4 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.
(Assignee)

Comment 7

4 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>
(Assignee)

Updated

4 years ago
Blocks: 1020438
(Assignee)

Comment 8

4 years ago
Created attachment 8434270 [details] [diff] [review]
ruleview-flicker.patch

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)
(Assignee)

Comment 9

4 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
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.