687698 - Integrate Style Editor's automatic transitions

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P2)

Comment 1

[Dave Camp (:dcamp)]

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

Comment 2

[Rob Campbell [:rc] (:robcee)]

filter on pegasus

Comment 3

[Cedric Vivier [:cedricv]]

Attachment: transition style sheet updates

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 588046 [details] [diff] [review]
transition style sheet updates

>+pref("devtools.styleeditor.transition_duration", 500);
>+pref("devtools.styleeditor.transition_function", "ease-out");

Why do we need prefs for this?

>+// observe StyleEditor prefs changes
>+Services.prefs.addObserver(PREF_BRANCH, {
>+  observe: function (aSubject, aTopic, aPrefName) {
>+    if (aTopic != "nsPref:changed") {
>+      return;
>+    }
>+    switch (aPrefName) {
>+      case TRANSITION_DURATION_PREF:
>+      case TRANSITION_FUNCTION_PREF:
>+        setTransitionDuration();
>+        break;
>+    }
>+  }
>+}, false);

And why do they need to be live?

Comment 5

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 588046 [details] [diff] [review]
> transition style sheet updates
> >+pref("devtools.styleeditor.transition_duration", 500);
> >+pref("devtools.styleeditor.transition_function", "ease-out");
> 
> Why do we need prefs for this?

Some users might prefer to work without transitions at all... or make them look a bit slower/quicker.


> >+// observe StyleEditor prefs changes
> >+Services.prefs.addObserver(PREF_BRANCH, {
> >+  observe: function (aSubject, aTopic, aPrefName) {
> >+    if (aTopic != "nsPref:changed") {
> >+      return;
> >+    }
> >+    switch (aPrefName) {
> >+      case TRANSITION_DURATION_PREF:
> >+      case TRANSITION_FUNCTION_PREF:
> >+        setTransitionDuration();
> >+        break;
> >+    }
> >+  }
> >+}, false);
> 
> And why do they need to be live?

It doesn't *need* to, but this doesn't seem very invasive for the convenience (the observer is only added the first time you launch Style Editor). Is it?

Comment 6

[Cedric Vivier [:cedricv]]

(In reply to Cedric Vivier [cedricv] from comment #5)
> Some users might prefer to work without transitions at all...

Just to clarify, if devtools.styleeditor.transition_duration is set to 0, transitions are disabled.

Comment 7

[Dão Gottwald [:dao]]

Sounds like there should just be a boolean pref to disable this.

Comment 8

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #7)
> Sounds like there should just be a boolean pref to disable this.

Maybe, but we'd then need to add a constant anyways, so it does not simplify things and reduce configurability needlessly IMHO (I guess setting slower transitions can be interesting for accessibility purposes as well as for personal taste)

Otoh, transition_function pref might be unnecessary I agree.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Cedric Vivier [cedricv] from comment #8)
> (In reply to Dão Gottwald [:dao] from comment #7)
> > Sounds like there should just be a boolean pref to disable this.
> 
> Maybe, but we'd then need to add a constant anyways, so it does not simplify
> things and reduce configurability needlessly IMHO (I guess setting slower
> transitions can be interesting for accessibility purposes as well as for
> personal taste)

For accessibility you just want to completely disable it as well... and I don't think it's ideal to expect users who want to disable this to change devtools.styleeditor.transition_duration.

Please don't add prefs for random stuff just because you can. Prefs should be there for us to easily disable or tune features or for users to adjust behavior where we expect a large enough need to not rely on add-ons. I don't see how the transition length is different from random other constants in our code.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 588046 [details] [diff] [review]
transition style sheet updates

 // Enable the Style Editor.
 pref("devtools.styleeditor.enabled", true);
+pref("devtools.styleeditor.transition_duration", 500);
+pref("devtools.styleeditor.transition_function", "ease-out");

I think we can make these as defaults. I'd say the transition function is certainly possible to set as a constant. The duration *might* be useful to tweak, but I'm kind of with Dão that we should pick sane values that work for everyone and live with them.

We should probably also have a single pref for disabling transitions entirely that isn't a numeric value.

+let gTransitionDuration;
+let gTransitionInsert;

these aren't really global, they're module-global. We can probably do away with the 'g' prefix (and make them consts in the process).

This means we don't need a template, don't need the setTransitionDuration function or the pref observer.

Otherwise, this is great! I look forward to a simplified patch. r+ with those changes.

Comment 11

[Cedric Vivier [:cedricv]]

Attachment: transition style sheet updates - one pref version with no pref observer

Comment 12

[Dão Gottwald [:dao]]

I'm not sure why robcee said "r+ with those changes" and canceled the request at the same time... Can you please get formal review?

Comment 13

[Rob Campbell [:rc] (:robcee)]

(In reply to Dão Gottwald [:dao] from comment #12)
> I'm not sure why robcee said "r+ with those changes" and canceled the
> request at the same time... Can you please get formal review?

I was being polite. R- is such a heavy thing. I figured Cedric would re-ask for review when he resubmitted.

I will look at this revised patch before end of day today, first glance looked promising!

Comment 14

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 588863 [details] [diff] [review]
transition style sheet updates - one pref version with no pref observer

in _onTransitionEnd:

+    if (aEvent) {
+      aEvent.stopPropagation();
+      aEvent.stopImmediatePropagation();
+    }

do we need both of these stopPropagation calls?

in any case, looks very fine!

Comment 15

[Rob Campbell [:rc] (:robcee)]

has this run through try yet? Let me know and if it's been through already, please add [land-in-fx-team] to the status whiteboard. thanks!

Comment 16

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #13)
> I was being polite. R- is such a heavy thing. I figured Cedric would re-ask
> for review when he resubmitted.

It is not necessary to be over-polite Rob ;)
I also got confused, I remember you once told me not to re-ask for review when you say "r+ with [insert trivial change here]" :p


(In reply to Rob Campbell [:rc] (robcee) from comment #14)
> Comment on attachment 588863 [details] [diff] [review]
> do we need both of these stopPropagation calls?

Oops. stopImmediatePropagation is enough indeed :)

(In reply to Rob Campbell [:rc] (robcee) from comment #15)
> has this run through try yet? Let me know and if it's been through already

Yup, all recent patches that brings Style Editor to add-on feature parity are green on try:
https://tbpl.mozilla.org/?tree=Try&rev=2c8d16d10eec

Comment 17

[Cedric Vivier [:cedricv]]

Attachment: v3 - remove redundant stopPropagation call

Comment 18

[Dão Gottwald [:dao]]

Please use stopPropagation instead of stopImmediatePropagation. (Unless there's a reason to use the latter here that I'm missing?)

Comment 19

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #18)
> Please use stopPropagation instead of stopImmediatePropagation. (Unless
> there's a reason to use the latter here that I'm missing?)

The Style Editor is done with it, the event doesn't need to be handled by anything else... therefore we can stopImmediatePropagation.

Comment 20

[Dão Gottwald [:dao]]

Is this supposed to prevent content scripts from getting the transitionend event?

Comment 21

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #20)
> Is this supposed to prevent content scripts from getting the transitionend
> event?

No.
This is supposed to remove the handler as soon as possible after the Style Editor is done with it... if you really prefer the use of stopPropagation for this I can post a patch ya.

Comment 22

[Dão Gottwald [:dao]]

Shouldn't you try to consume the event before content? For that I think you want to attach a capturing listener to the document, and call stopImmediatePropagation in case preventing content from consuming the event is wanted. Otherwise it seems to me that  propagation shouldn't be stopped at all.

Comment 23

[Cedric Vivier [:cedricv]]

Attachment: v3 - prefer capturing listener on transitionend

Comment 24

[Cedric Vivier [:cedricv]]

(In reply to Dão Gottwald [:dao] from comment #22)
> For that I think you
> want to attach a capturing listener to the document, and call
> stopImmediatePropagation in case preventing content from consuming the event
> is wanted.

Makes sense. Updated patch to use capturing listener on defaultView instead.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 589163 [details] [diff] [review]
v3 - prefer capturing listener on transitionend

>+  _onTransitionEnd: function SE__onTransitionEnd(aEvent)
>+  {
>+    let content = this.contentDocument;
>+    let root = content.documentElement;
>+    if (!this._transitionRefCount ||
>+        (aEvent && aEvent.target != root) ||
>+        !root.classList.contains(TRANSITION_CLASS)) {
>+      return false; // not a StyleEditor-generated transition

Why false?

>+    if (!this._transitionRefCount) {
>+      if (aEvent) {
>+        aEvent.stopImmediatePropagation();
>+      }

Why is this now dependent on this._transitionRefCount?

Comment 26

[Rob Campbell [:rc] (:robcee)]

makes sense. Good suggestions, Dao.

Cedric, in the future I will stop being polite. :)

Can't wait to get this in.

Comment 27

[Cedric Vivier [:cedricv]]

Attachment: v4 - just remove unnecessary event handling

So, looking back at this... I end up thinking the whole event handling is kinda unnecessary complexity anyways - we can just use the timer-based fallback every time.

Updated patch... on the right bug this time :/

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 589450 [details] [diff] [review]
v4 - just remove unnecessary event handling

>+const TRANSITION_DURATION_MS = 500; /* sync with TRANSITION_RULE */
>+const TRANSITION_RULE = "\
>+:root.moz-styleeditor-transitioning, :root.moz-styleeditor-transitioning * {\
>+-moz-transition-duration: 500ms !important; \

-moz-transition-duration: " + TRANSITION_DURATION_MS + "ms !important; \

and drop the /* sync with TRANSITION_RULE */ comment

>+  this._onTransitionEndBinding = this._onTransitionEnd.bind(this);

remove this

>+    content.defaultView.setTimeout(this._onTransitionEndBinding,

and write this._onTransitionEnd.bind(this) here

Comment 29

[Cedric Vivier [:cedricv]]

Attachment: v4 + Dao comments applied

Comment 30

[Rob Campbell [:rc] (:robcee)]

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

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 32

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 589832 [details] [diff] [review]
v4 + Dao comments applied

[Approval Request Comment]
Regression caused by (bug #): New feature. Not a regression.
User impact if declined: User won't have this awesome feature.
Testing completed (on m-c, etc.): m-c.
Risk to taking this patch (and alternatives if risky): New code always adds risk. Introduces new behavior that was originally intended to ship with this feature. It makes the Style Editor significantly better.

Comment 33

[Alex Keybl [:akeybl]]

Comment on attachment 589832 [details] [diff] [review]
v4 + Dao comments applied

[Triage Comment]
This feels like post-landing feature work which should instead ride the train. Users will first see this in FF12.