Last Comment Bug 687698 - Integrate Style Editor's automatic transitions
: Integrate Style Editor's automatic transitions
Status: RESOLVED FIXED
[styleeditor]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Style Editor (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 12
Assigned To: Cedric Vivier [:cedricv]
:
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Mentors:
Depends on: 583041
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-19 15:57 PDT by Cedric Vivier [:cedricv]
Modified: 2012-02-06 11:44 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
transition style sheet updates (12.29 KB, patch)
2012-01-12 08:41 PST, Cedric Vivier [:cedricv]
no flags Details | Diff | Splinter Review
transition style sheet updates - one pref version with no pref observer (11.40 KB, patch)
2012-01-16 06:37 PST, Cedric Vivier [:cedricv]
rcampbell: review+
Details | Diff | Splinter Review
v3 - remove redundant stopPropagation call (11.33 KB, patch)
2012-01-16 20:22 PST, Cedric Vivier [:cedricv]
no flags Details | Diff | Splinter Review
v3 - prefer capturing listener on transitionend (11.42 KB, patch)
2012-01-17 05:53 PST, Cedric Vivier [:cedricv]
no flags Details | Diff | Splinter Review
v4 - just remove unnecessary event handling (9.83 KB, patch)
2012-01-18 03:05 PST, Cedric Vivier [:cedricv]
dao+bmo: review+
Details | Diff | Splinter Review
v4 + Dao comments applied (9.77 KB, patch)
2012-01-19 04:30 PST, Cedric Vivier [:cedricv]
akeybl: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description Cedric Vivier [:cedricv] 2011-09-19 15:57:24 PDT

    
Comment 1 Dave Camp (:dcamp) 2011-10-27 09:05:30 PDT
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Comment 2 Rob Campbell [:rc] (:robcee) 2012-01-10 08:22:28 PST
filter on pegasus
Comment 3 Cedric Vivier [:cedricv] 2012-01-12 08:41:59 PST
Created attachment 588046 [details] [diff] [review]
transition style sheet updates
Comment 4 Dão Gottwald [:dao] 2012-01-12 09:30:21 PST
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] 2012-01-12 10:41:05 PST
(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] 2012-01-12 10:42:27 PST
(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] 2012-01-12 11:01:44 PST
Sounds like there should just be a boolean pref to disable this.
Comment 8 Cedric Vivier [:cedricv] 2012-01-13 02:46:06 PST
(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] 2012-01-13 03:56:48 PST
(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) 2012-01-13 11:02:41 PST
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] 2012-01-16 06:37:07 PST
Created attachment 588863 [details] [diff] [review]
transition style sheet updates - one pref version with no pref observer
Comment 12 Dão Gottwald [:dao] 2012-01-16 07:47:46 PST
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) 2012-01-16 11:18:57 PST
(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) 2012-01-16 11:51:04 PST
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) 2012-01-16 12:59:04 PST
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] 2012-01-16 20:21:35 PST
(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] 2012-01-16 20:22:50 PST
Created attachment 589095 [details] [diff] [review]
v3 - remove redundant stopPropagation call
Comment 18 Dão Gottwald [:dao] 2012-01-17 00:43:17 PST
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] 2012-01-17 01:04:35 PST
(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] 2012-01-17 01:07:00 PST
Is this supposed to prevent content scripts from getting the transitionend event?
Comment 21 Cedric Vivier [:cedricv] 2012-01-17 01:11:51 PST
(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] 2012-01-17 01:33:10 PST
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] 2012-01-17 05:53:34 PST
Created attachment 589163 [details] [diff] [review]
v3 - prefer capturing listener on transitionend
Comment 24 Cedric Vivier [:cedricv] 2012-01-17 05:54:08 PST
(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] 2012-01-17 06:02:20 PST
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) 2012-01-17 12:02:04 PST
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] 2012-01-18 03:05:32 PST
Created attachment 589450 [details] [diff] [review]
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] 2012-01-19 03:33:54 PST
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] 2012-01-19 04:30:09 PST
Created attachment 589832 [details] [diff] [review]
v4 + Dao comments applied
Comment 30 Rob Campbell [:rc] (:robcee) 2012-01-19 07:10:28 PST
https://hg.mozilla.org/integration/fx-team/rev/c2526ed5efda
Comment 31 Tim Taubert [:ttaubert] 2012-01-19 15:49:09 PST
https://hg.mozilla.org/mozilla-central/rev/c2526ed5efda
Comment 32 Rob Campbell [:rc] (:robcee) 2012-01-23 07:19:05 PST
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] 2012-01-23 09:10:19 PST
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.

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