The default bug view has changed. See this FAQ.

Integrate Style Editor's automatic transitions

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Developer Tools: Style Editor
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cedricv, Assigned: cedricv)

Tracking

unspecified
Firefox 12
Points:
---

Firefox Tracking Flags

(firefox11-)

Details

(Whiteboard: [styleeditor])

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

6 years ago
Summary: Integrate Style Editor's automatic transitioning → Integrate Style Editor's automatic transitions

Comment 1

6 years ago
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Priority: -- → P2
filter on pegasus
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 588046 [details] [diff] [review]
transition style sheet updates
Attachment #588046 - Flags: review?
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?
(Assignee)

Comment 5

5 years ago
(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?
(Assignee)

Comment 6

5 years ago
(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.
Sounds like there should just be a boolean pref to disable this.
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #588046 - Flags: review? → review?(rcampbell)
(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 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.
Attachment #588046 - Flags: review?(rcampbell)
(Assignee)

Comment 11

5 years ago
Created attachment 588863 [details] [diff] [review]
transition style sheet updates - one pref version with no pref observer
Attachment #588046 - Attachment is obsolete: true
Attachment #588863 - Flags: superreview-
(Assignee)

Updated

5 years ago
Attachment #588863 - Flags: superreview-
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
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?
Whiteboard: [land-in-fx-team]
(Assignee)

Updated

5 years ago
Attachment #588863 - Flags: review?(rcampbell)
(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 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!
Attachment #588863 - Flags: review?(rcampbell) → review+
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!
(Assignee)

Comment 16

5 years ago
(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
(Assignee)

Comment 17

5 years ago
Created attachment 589095 [details] [diff] [review]
v3 - remove redundant stopPropagation call
Attachment #588863 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Assignee: cedricv → nobody
Component: Developer Tools → Developer Tools: Style Editor
QA Contact: developer.tools → developer.tools.style.editor
Whiteboard: [styleeditor][land-in-fx-team]
Please use stopPropagation instead of stopImmediatePropagation. (Unless there's a reason to use the latter here that I'm missing?)
Whiteboard: [styleeditor][land-in-fx-team] → [styleeditor]

Updated

5 years ago
Assignee: nobody → cedricv
(Assignee)

Comment 19

5 years ago
(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.
Is this supposed to prevent content scripts from getting the transitionend event?
(Assignee)

Comment 21

5 years ago
(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.
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.
(Assignee)

Comment 23

5 years ago
Created attachment 589163 [details] [diff] [review]
v3 - prefer capturing listener on transitionend
Attachment #589095 - Attachment is obsolete: true
Attachment #589163 - Flags: review?(dao)
(Assignee)

Comment 24

5 years ago
(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 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?
makes sense. Good suggestions, Dao.

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

Can't wait to get this in.
(Assignee)

Comment 27

5 years ago
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 :/
Attachment #589163 - Attachment is obsolete: true
Attachment #589163 - Flags: review?(dao)
Attachment #589450 - Flags: review?(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
Attachment #589450 - Flags: review?(dao) → review+
(Assignee)

Comment 29

5 years ago
Created attachment 589832 [details] [diff] [review]
v4 + Dao comments applied
Attachment #589450 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [styleeditor] → [styleeditor][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/c2526ed5efda
Whiteboard: [styleeditor][land-in-fx-team] → [styleeditor][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c2526ed5efda
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [styleeditor][fixed-in-fx-team] → [styleeditor]
Target Milestone: --- → Firefox 12
tracking-firefox11: --- → ?
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.
Attachment #589832 - Flags: approval-mozilla-aurora?
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.
Attachment #589832 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

5 years ago
tracking-firefox11: ? → -
You need to log in before you can comment on or make changes to this bug.