692466 - [highlighter] transitions should be disabled only while scrolling, not when the node is locked

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Paul Rouget [:paul]]

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]

is this a dupe of bug 689934?

Comment 2

[Paul Rouget [:paul]]

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

Also, the current behavior doesn't prevent transitions if the user is scrolling while the node is not locked.

Comment 4

[Dave Camp (:dcamp)]

We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.

Comment 5

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 575987 [details] [diff] [review]
patch v1

Please use set/removeAttribute instead of classList.add/remove here.

Comment 7

[Joe Walker [:jwalker]]

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

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

Attachment: patch v1.1

attribute instead of class

Comment 10

[Mihai Sucan [:msucan]]

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

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

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/cc0ce6c25760