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

Status: RESOLVED FIXED

(DevTools :: Style Editor, enhancement, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

K, thanks a lot jryans, will give it a shot tomorrow!

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1396099: Insert the transition sheet for the style editor in the UA level.

Review commit: https://reviewboard.mozilla.org/r/186428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186428/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1396099: Make :-moz-styleeditor-transitioning only valid in UA sheets.

Review commit: https://reviewboard.mozilla.org/r/186430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186430/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8915188 [details]
Bug 1396099: Make :-moz-styleeditor-transitioning only valid in UA sheets.

https://reviewboard.mozilla.org/r/186430/#review191638

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(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 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8ebc51bb8bd0
https://hg.mozilla.org/mozilla-central/rev/aa679e64c271

Comment 12

[Chris Mills [:cmills]]

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