Closed Bug 1396099 Opened 2 years ago Closed 2 years ago

Insert :-moz-styleeditor-transitioning using a UA sheet, and hide it from content pages

Categories

(DevTools :: Style Editor, enhancement, P3)

enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: jryans, Assigned: emilio)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files)

The style editor makes use of the pseudo-class `:-moz-styleeditor-transitioning` to animate changes as you edit.

It currently inserts a rule with this psuedo-class into the author sheet being edited.  This means it's currently exposed to content as well.

We should change to style editor to insert this with a separate UA sheet, so we can lock down the pseudo-class and stop exposing it to content.
Priority: -- → P3
No longer blocks: 1396073
ni? so jryans can take a look when he can at this. I'd like to get this fixed. I can try to fix but I'm not sure how to test it works.
Flags: needinfo?(jryans)
Summary: Insert :-moz-styleeditor-transitioning using a UA sheet → Insert :-moz-styleeditor-transitioning using a UA sheet, and hide it from content pages
When you use the Style Editor to modify styles (this is full panel named "Style Editor" that shows whole stylesheets, not something in the Inspector), the changes you make are animated using a transition.

As a quick manual check, you can append something like `body { background: blue }` to the bottom of a sheet.  Or, even more obvious, just select all and cut the entire sheet.  Many things will animate!

This is done by inserting a rule[1] based on this pseudo-class.  Effectively, we do:

1. Modify the sheet with the new text
2. Enable the pseudo-class
3. Insert rule using pseudo-class[2]
4. Undo pseudo-class and rule once transition is done

At the moment, the rule is inserted into whatever sheet you modify in the editor (that's the `rawSheet` object).

You change this approach to instead add a separate UA sheet to hold the rule.  For example:

1. Move the transition rule to some new CSS file and add it to moz.build
2. Use the loadSheet helper[3] to load it as a UA sheet (resource://devtools/path/to/file)
  * `this.window` should get you the window object to pass here
3. Remove the sheet where the rule was removed before

Let me know if you have more questions!

[1]: http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/devtools/server/actors/stylesheets.js#34-39
[2]: http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/devtools/server/actors/stylesheets.js#746
[3]: http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/devtools/shared/layout/utils.js#728-772
Flags: needinfo?(jryans)
K, thanks a lot jryans, will give it a shot tomorrow!
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Comment on attachment 8915188 [details]
Bug 1396099: Make :-moz-styleeditor-transitioning only valid in UA sheets.

https://reviewboard.mozilla.org/r/186430/#review191638
Attachment #8915188 - Flags: review?(xidorn+moz) → review+
The intent to unship for this is at https://groups.google.com/forum/#!topic/mozilla.dev.platform/mVFD29YcVEc

That email doesn't mention this bug, though.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> The intent to unship for this is at
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/mVFD29YcVEc
> 
> That email doesn't mention this bug, though.

Yeah, originally this was going to be only about part 1, and then bug 1396073 about unshipping all the stuff. But then went ahead with the easy ones in that one, and repurposed this to also unship it.
Comment on attachment 8915187 [details]
Bug 1396099: Insert the transition sheet for the style editor in the UA level.

https://reviewboard.mozilla.org/r/186428/#review191744

Thanks for working on this! :) The previous functionality appears to be preserved in my local testing.

::: devtools/server/actors/stylesheets.js:43
(Diff revision 1)
> -  transition-duration: ${TRANSITION_DURATION_MS}ms !important;
> +    transition-duration: ${TRANSITION_DURATION_MS}ms !important;
> -  transition-delay: 0ms !important;
> +    transition-delay: 0ms !important;
> -  transition-timing-function: ease-out !important;
> +    transition-timing-function: ease-out !important;
> -  transition-property: all !important;
> +    transition-property: all !important;
> -}`;
> +  }
> +`)

Nit: needs ;

::: devtools/server/actors/stylesheets.js:752
(Diff revision 1)
> -    addPseudoClassLock(this.document.documentElement, TRANSITION_PSEUDO_CLASS);
> +    if (!this._transitionSheetLoaded) {
> +      this._transitionSheetLoaded = true;
> +      // We don't remove this sheet. It uses an internal selector that
> +      // we only apply via locks, so there's no need to load and unload
> +      // it all the time.
> +      loadSheet(this.window, TRANSITION_SHEET);

Looks like it's possible for this load call to happen multiple times, since we here we track state per sheet, but really loading a sheet is a document level operation.  Also, we get fresh actor objects with each DevTools session, so that could trigger additional loads.

But for practical purposes it seems fine because the actual `Document::LoadAdditionalStyleSheet`[1] checks first to see if the sheet was already loaded and aborts if it was, so that seems good enough!

[1]: http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/base/nsDocument.cpp#4733
Attachment #8915187 - Flags: review?(jryans) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8ebc51bb8bd0
Insert the transition sheet for the style editor in the UA level. r=jryans
https://hg.mozilla.org/integration/autoland/rev/aa679e64c271
Make :-moz-styleeditor-transitioning only valid in UA sheets. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/8ebc51bb8bd0
https://hg.mozilla.org/mozilla-central/rev/aa679e64c271
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
We never documented this pseudo-class, so I've just added a note to the Fx58 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/58#CSS_2
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.