Last Comment Bug 692466 - [highlighter] transitions should be disabled only while scrolling, not when the node is locked
: [highlighter] transitions should be disabled only while scrolling, not when t...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 11
Assigned To: Paul Rouget [:paul]
:
: Patrick Brosset <:pbro>
Mentors:
Depends on:
Blocks: 663830
  Show dependency treegraph
 
Reported: 2011-10-06 08:53 PDT by Paul Rouget [:paul]
Modified: 2011-11-29 22:05 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (4.14 KB, patch)
2011-11-21 15:09 PST, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.1 (4.15 KB, patch)
2011-11-28 00:51 PST, Paul Rouget [:paul]
dao+bmo: review+
mihai.sucan: review+
Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-10-06 08:53:08 PDT
We disable the transitions of the different highlighter elements once a node is locked. So we see these transitions only while inspecting. We don't see them when the HTML Tree view and the Breadcrumbs display are used.

So we need to disable transitions while scrolling and resizing.
I would propose that: onscroll, we disable the transitions for 200ms. If the user stops scrolling, the transitions are back. If he continues scrolling, we re-disable the transitions for 200ms, etc… (same for resizing).
Comment 1 Kevin Dangoor 2011-10-06 08:58:19 PDT
is this a dupe of bug 689934?
Comment 2 Paul Rouget [:paul] 2011-10-06 09:31:45 PDT
I don't think so. Bug 689934 is about the infobar's transitions not being disabled. Here I talk about when to disable transitions.
Comment 3 Paul Rouget [:paul] 2011-10-17 05:31:41 PDT
Also, the current behavior doesn't prevent transitions if the user is scrolling while the node is not locked.
Comment 4 Dave Camp (:dcamp) 2011-10-27 08:40:37 PDT
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Comment 5 Paul Rouget [:paul] 2011-11-21 15:09:44 PST
Created attachment 575987 [details] [diff] [review]
patch v1
Comment 6 Dão Gottwald [:dao] 2011-11-26 04:40:52 PST
Comment on attachment 575987 [details] [diff] [review]
patch v1

Please use set/removeAttribute instead of classList.add/remove here.
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-26 05:53:39 PST
(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 575987 [details] [diff] [review] [diff] [details] [review]
> patch v1
> 
> Please use set/removeAttribute instead of classList.add/remove here.

What's up with classList.add/remove?
Comment 8 Paul Rouget [:paul] 2011-11-28 00:28:21 PST
(In reply to Joe Walker from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > Comment on attachment 575987 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch v1
> > 
> > Please use set/removeAttribute instead of classList.add/remove here.
> 
> What's up with classList.add/remove?

Dao meant that it's better to use an attribute than a class here.
If I'm not mistaken, classes should be re-usable.

I will use a disable-transitions attribute.
Comment 9 Paul Rouget [:paul] 2011-11-28 00:51:44 PST
Created attachment 577203 [details] [diff] [review]
patch v1.1

attribute instead of class
Comment 10 Mihai Sucan [:msucan] 2011-11-28 11:14:39 PST
Comment on attachment 577203 [details] [diff] [review]
patch v1.1

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

Patch looks and works good. Thank you!

::: browser/devtools/highlighter/inspector.jsm
@@ +699,5 @@
> +     this.IUI.win.setTimeout(function() {
> +       this.veilContainer.removeAttribute("disable-transitions");
> +       this.nodeInfo.container.removeAttribute("disable-transitions");
> +       this.transitionDisabler = null;
> +     }.bind(this), 500);

You might want to put the number in a constant at the top of the JSM.
Comment 11 Rob Campbell [:rc] (:robcee) 2011-11-29 11:12:59 PST
https://hg.mozilla.org/integration/fx-team/rev/cc0ce6c25760
Comment 12 Tim Taubert [:ttaubert] 2011-11-29 22:05:39 PST
https://hg.mozilla.org/mozilla-central/rev/cc0ce6c25760

Note You need to log in before you can comment on or make changes to this bug.