Closed Bug 1040882 Opened 6 years ago Closed 5 years ago

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

Categories

(DevTools :: Style Editor, defect, P1)

x86_64
All
defect

Tracking

(firefox33 fixed, firefox34 fixed)

RESOLVED FIXED
Firefox 34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: paul, Assigned: harth)

References

Details

Attachments

(1 file, 3 obsolete files)

Anything I change in a stylesheet get dismissed after a couple of seconds.
This is happening at least on Firefox OS. I have 2 devices, happens on both.
    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.
Priority: -- → P1
Assignee: nobody → fayearthur
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.
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.
Attachment #8465073 - Flags: review?(jwalker)
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.
Attachment #8465073 - Attachment is obsolete: true
Attachment #8465073 - Flags: review?(jwalker)
Attachment #8465086 - Flags: review?(jwalker)
Last one. Fix for try run failure.
Attachment #8465086 - Attachment is obsolete: true
Attachment #8465086 - Flags: review?(jwalker)
Attachment #8465215 - Flags: review?(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.
Attachment #8465215 - Flags: review?(jwalker) → review+
Good point, this patch just cancels the timeout.

Try: https://tbpl.mozilla.org/?tree=Try&rev=d36cd55f54b7
Attachment #8465215 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/b463979c8dd3
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b463979c8dd3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
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
Attachment #8465649 - Flags: approval-mozilla-aurora?
Attachment #8465649 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1072715
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.