Closed
Bug 966242
Opened 11 years ago
Closed 10 years ago
[ruleview] Refreshing the rule view has a delay and causes a flicker
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: bgrins, Assigned: bgrins)
References
(Blocks 2 open bugs, )
Details
Attachments
(1 file, 1 obsolete file)
2.35 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
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•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
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.
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.
Assignee | ||
Updated•10 years ago
|
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
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
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.
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>
Assignee | ||
Comment 8•10 years ago
|
||
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•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
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.
Attachment #8434270 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/65b0d08a49ac
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/65b0d08a49ac
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•