1040882 - [style editor] Changes are dismissed after a couple of seconds

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P1)

Description

[Paul Rouget [:paul]]

Anything I change in a stylesheet get dismissed after a couple of seconds.

Comment 1

[Paul Rouget [:paul]]

This is happening at least on Firefox OS. I have 2 devices, happens on both.

Comment 2

[nicola greco]

    I connect to Firefox Desktop with https://github.com/paulrouget/firefox-remote-styleEditors/blob/master/libs/client.py

    And I send the following packages:

    DBG-SERVER: New debugging connection on 127.0.0.1:6080
    DBG-SERVER: Got: {
      "to": "root",
      "type": "listTabs"
    }
    DBG-SERVER: Got: {
      "to": "conn0.stylesheets98",
      "type": "getStyleSheets"
    }
    DBG-SERVER: Got: {
      "to": "conn0.stylesheet109",
      "type": "getText"
    }
    DBG-SERVER: Got: {
      "to": "conn0.stylesheet109",
      "transition": true,
      "type": "update",
      "text": "body {background:#000;}\n"
    }

    The page I am in, in this case http://nicolagreco.com/test gets black, but after a couple of seconds, it turns white again.

Comment 3

[Heather Arthur [:harth]]

I bisected incorrectly at least once because this isn't super consistent, but I think this range might be correct:

Last good nightly: 2014-03-03
First bad nightly: 2014-03-04

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4cb766685b73&tochange=529b86b92b1d

The obvious choice from that is bug 978653. I'm thinking it's a race condition where instead of removing the transition rule, we remove an actual rule.

Comment 4

[Heather Arthur [:harth]]

Attachment: Don't delete the last rule unless it's the transition rule we added

Whoa. I think this bug has been here since the beginning of the Style Editor, but was just made worse by bug 978653.

If you edit the sheet twice in succession what happens is we 1) Add a transition rule 2) Replace all the rules, getting rid of the transition rule 3) Go around to remove the transition rule, but it's not there anymore so we remove a real rule.

So this patch will check to make sure the rule we're removing is the transition rule that we added. The test catches this error.

Comment 5

[Heather Arthur [:harth]]

Attachment: Always add transition rule and only delete transition rule

This patch is updated to fix another bug.

Since we blow away all the rules on every update, we're also blowing away the transition rule. Therefore we need to add the transition rule to the end of the sheet on every update. Before we were doing some ref counting so that we didn't add it every time.

Comment 6

[Heather Arthur [:harth]]

Attachment: Always add transition rule and only delete transition rule, +fix

Last one. Fix for try run failure.

Comment 7

[Joe Walker [:jwalker]]

Comment on attachment 8465215 [details] [diff] [review]
Always add transition rule and only delete transition rule, +fix

Review of attachment 8465215 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/stylesheets.js
@@ +915,3 @@
>        this.document.documentElement.classList.add(TRANSITION_CLASS);
>      }
> +    this._transitionRefCount++;

An alternative to reference counting might be to cancel and reset the timeout each time _insertTransistionRule is called. That way we only call _onTransitionEnd when we're done. Feels slightly nicer, but not sure it's worth the effort since this works.

@@ +937,5 @@
> +      let index = this.rawSheet.cssRules.length - 1;
> +      let rule = this.rawSheet.cssRules[index];
> +      if (rule.selectorText == TRANSITION_RULE_SELECTOR) {
> +        this.rawSheet.deleteRule(index);
> +      }

Maybe we should loop over them all removing any that match TRANSITION_RULE_SELECTOR?

@@ +942,3 @@
>      }
>  
>      events.emit(this, "style-applied");

if (--this._transitionRefCount > 0) then we've not changed the rules, so shouldn't this be in the if block?
Then we wouldn't need the additional ref-counting in the test?

Perhaps this will break other things though.

Comment 8

[Heather Arthur [:harth]]

Attachment: Updated to check in

Good point, this patch just cancels the timeout.

Try: https://tbpl.mozilla.org/?tree=Try&rev=d36cd55f54b7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b463979c8dd3

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b463979c8dd3

Comment 11

[Heather Arthur [:harth]]

Comment on attachment 8465649 [details] [diff] [review]
Updated to check in

Approval Request Comment
[Feature/regressing bug #]: bug 978653

[User impact if declined]:
Often editing a style sheet in the style editor will cause some page styles to be lost.

[Describe test coverage new/current, TBPL]:
This patch has a test and has been in mozilla-central for a few days.

[Risks and why]: N/A

[String/UUID change made/needed]: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d5e139b2ef95