935176 - saving a style sheet doesn't remove the star

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Paul Rouget [:paul]]

STR:
- open the style editor
- import a style sheet
- do some changes
- see the * that indicates there's something to save
- save
- file is saved, but the star doesn't go away

Comment 1

[Paul Rouget [:paul]]

Probably a Firefox 27 regression.

Comment 2

[Heather Arthur [:harth]]

Bet it's from CodeMirror migration.

Comment 3

[Girish Sharma [:Optimizer]]

I wonder why no tests broke ...
and right now browser debugger is broken to get to the root of it .. :(

Comment 4

[Girish Sharma [:Optimizer]]

Attachment: patch

added a new test to test the presence of start and dirty state of the editor.

try at : https://tbpl.mozilla.org/?tree=Try&rev=a9110d88bfaf

Comment 5

[Girish Sharma [:Optimizer]]

(try is green)

Comment 6

[Girish Sharma [:Optimizer]]

(review ping)
I want this to be lifted to aurora before the merge :)

Comment 7

[Heather Arthur [:harth]]

Attachment: try this, fix display name bug as well

In the process of trying out this patch I noticed an even deeper problem. Saving a stylesheet to file doesn't update the display name (or the star).

1) Open style editor
2) Save the first style sheet to a file "areghjie.css" on disk
3) Name isn't updated to "areghjie.css" and star doesn't go away after saving it.

This patch seems to fix both problems, and they seem to be part of the same issue. Does this patch work for you Girish?

Comment 8

[Heather Arthur [:harth]]

Comment on attachment 8340682 [details] [diff] [review]
patch

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

Take that back, this will resolve the filename issue I was seeing as well. Cheers.

::: browser/devtools/sourceeditor/editor.js
@@ +236,5 @@
>  
>        cm.on("focus", () => this.emit("focus"));
>        cm.on("scroll", () => this.emit("scroll"));
>        cm.on("change", () => this.emit("change"));
> +      cm.on("change", () => {

instead of having two "change" listeners, combine?

::: browser/devtools/styleeditor/test/browser_styleeditor_filesave.js
@@ +43,5 @@
>  {
> +  editor.sourceEditor.once("dirty-change", () => {
> +    is(editor.sourceEditor.isClean(), false, "Editor is dirty.");
> +    ok(gUI._view.getSummaryElementByOrdinal(0).classList.contains("unsaved"),
> +       "Star icon is present in the corresponding summary.");

`editor.summary` should work instead of `gUI._view.getSummaryElementByOrdinal(0)`.

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: comments addressed and landed

Addressed review comments and landed in fx-team

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

Comment 10

[Girish Sharma [:Optimizer]]

Comment on attachment 8343860 [details] [diff] [review]
comments addressed and landed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 919978
User impact if declined: NO UI feedback to tell the user whether the file was saved or not. User is left confused as the file is actually saved but the Style Editor says it is not.
Testing completed (on m-c, etc.): Local, fx-team, added new tests.
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9408665d5384

Comment 12

[Girish Sharma [:Optimizer]]

Comment on attachment 8343860 [details] [diff] [review]
comments addressed and landed

Since during the aurora request only, merge happened, this is needed in beta too :)

Approval Request Comment]
Bug caused by (feature/regressing bug #): 919978
User impact if declined: NO UI feedback to tell the user whether the file was saved or not. User is left confused as the file is actually saved but the Style Editor says it is not.
Testing completed (on m-c, etc.): Local, fx-team, added new tests.
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 13

[Girish Sharma [:Optimizer]]

Sheriffs, please land this to aurora and beta whenever the next landings go :)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6ee992d5ae29