Closed
Bug 687698
Opened 13 years ago
Closed 13 years ago
Integrate Style Editor's automatic transitions
Categories
(DevTools :: Style Editor, defect, P2)
DevTools
Style Editor
Tracking
(firefox11-)
RESOLVED
FIXED
Firefox 12
Tracking | Status | |
---|---|---|
firefox11 | - | --- |
People
(Reporter: cedricv, Assigned: cedricv)
References
Details
(Whiteboard: [styleeditor])
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
akeybl
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•13 years ago
|
Summary: Integrate Style Editor's automatic transitioning → Integrate Style Editor's automatic transitions
Comment 1•13 years ago
|
||
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Priority: -- → P2
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #588046 -
Flags: review?
Comment 4•13 years ago
|
||
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•13 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•13 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.
Comment 7•13 years ago
|
||
Sounds like there should just be a boolean pref to disable this.
Assignee | ||
Comment 8•13 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•13 years ago
|
Attachment #588046 -
Flags: review? → review?(rcampbell)
Comment 9•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
Attachment #588046 -
Attachment is obsolete: true
Attachment #588863 -
Flags: superreview-
Assignee | ||
Updated•13 years ago
|
Attachment #588863 -
Flags: superreview-
Assignee | ||
Updated•13 years ago
|
Whiteboard: [land-in-fx-team]
Comment 12•13 years ago
|
||
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•13 years ago
|
Attachment #588863 -
Flags: review?(rcampbell)
Comment 13•13 years ago
|
||
(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•13 years ago
|
||
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+
Comment 15•13 years ago
|
||
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•13 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•13 years ago
|
||
Attachment #588863 -
Attachment is obsolete: true
Assignee | ||
Updated•13 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]
Comment 18•13 years ago
|
||
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•13 years ago
|
Assignee: nobody → cedricv
Assignee | ||
Comment 19•13 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.
Comment 20•13 years ago
|
||
Is this supposed to prevent content scripts from getting the transitionend event?
Assignee | ||
Comment 21•13 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.
Comment 22•13 years ago
|
||
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•13 years ago
|
||
Attachment #589095 -
Attachment is obsolete: true
Attachment #589163 -
Flags: review?(dao)
Assignee | ||
Comment 24•13 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 25•13 years ago
|
||
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•13 years ago
|
||
makes sense. Good suggestions, Dao.
Cedric, in the future I will stop being polite. :)
Can't wait to get this in.
Assignee | ||
Comment 27•13 years ago
|
||
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 28•13 years ago
|
||
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•13 years ago
|
||
Attachment #589450 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Whiteboard: [styleeditor] → [styleeditor][land-in-fx-team]
Comment 30•13 years ago
|
||
Whiteboard: [styleeditor][land-in-fx-team] → [styleeditor][fixed-in-fx-team]
Comment 31•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [styleeditor][fixed-in-fx-team] → [styleeditor]
Target Milestone: --- → Firefox 12
Updated•13 years ago
|
tracking-firefox11:
--- → ?
Comment 32•13 years ago
|
||
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 33•13 years ago
|
||
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•13 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•