Closed Bug 935176 Opened 6 years ago Closed 6 years ago

saving a style sheet doesn't remove the star

Categories

(DevTools :: Style Editor, defect)

x86
All
defect
Not set

Tracking

(firefox27 fixed, firefox28 fixed)

RESOLVED FIXED
Firefox 28
Tracking Status
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: paul, Assigned: Optimizer)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

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
Probably a Firefox 27 regression.
Bet it's from CodeMirror migration.
I wonder why no tests broke ...
and right now browser debugger is broken to get to the root of it .. :(
Attached patch patch (obsolete) — Splinter Review
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
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attachment #8340682 - Flags: review?(fayearthur)
(try is green)
(review ping)
I want this to be lifted to aurora before the merge :)
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?
Attachment #8343559 - Flags: feedback?
Attachment #8343559 - Flags: feedback? → feedback?(scrapmachines)
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)`.
Attachment #8340682 - Flags: review?(fayearthur) → review+
Attachment #8343559 - Attachment is obsolete: true
Attachment #8343559 - Flags: feedback?(scrapmachines)
Addressed review comments and landed in fx-team

https://hg.mozilla.org/integration/fx-team/rev/9408665d5384
Attachment #8340682 - Attachment is obsolete: true
Attachment #8343860 - Flags: review+
Whiteboard: [fixed-in-fx-team]
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
Attachment #8343860 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/9408665d5384
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Attachment #8343860 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8343860 - Flags: approval-mozilla-beta?
Attachment #8343860 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Sheriffs, please land this to aurora and beta whenever the next landings go :)
Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.