826982 - [style editor] Show a notification when navigating away from page if there are unsaved changes

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Heather Arthur [:harth]]

It would be nice to reapply Style Editor changes when reloading the page. However, in the interim, I propose we at least show a notification if the user tries to reload or navigate and there are unsaved changes in the Style Editor.

Similar to what the inspector does, this notification would allow the user to stay on the page and give them a chance to save, or continue navigating and lose their changes.

Comment 1

[Heather Arthur [:harth]]

Attachment: Show notification before page navigates if Style Editor has unsaved changes

Comment 2

[Paul Rouget [:paul]]

I'd like to discuss this change:
- don't we want to merge these kind of notifications into a generic one? Because if we make a change in the inspector and the style editor, we will show 2 notifications.
- these notification are annoying. We are already getting complaints about the inspector being annoying with its notifications.

I'd like to have a better generic mechanism that show a popup for all the tools. Maybe located in the toolbox. Before closing, the toolbox could ask all the tools if they have unsaved changes, and if one or more are dirty, it will focus the first dirty tool and show one notification. This notification should also have a "don't ask again for this site" button. See bug 818754.

Comment 3

[Heather Arthur [:harth]]

I can see the Inspector notification being annoying, especially given that you can't save the changes anyways. IMO, we should get rid of the Inspector notification. For the Style Editor, I'd really want to know if I hadn't saved changes back to disk. I've tried it out with this and I really like having it there.

I just saw this as a stop-gap for 20.

Comment 4

[Paul Rouget [:paul]]

(In reply to Heather Arthur [:harth] from comment #3)
> I can see the Inspector notification being annoying, especially given that
> you can't save the changes anyways. IMO, we should get rid of the Inspector
> notification.

The notification is here mostly to avoid leaving the page by mistake (clicking on a link while inspecting for example). So it's needed. But we need a way to get rid of it (bug 818754).

> For the Style Editor, I'd really want to know if I hadn't
> saved changes back to disk. I've tried it out with this and I really like
> having it there.

Would a global notification system work for you? I think it would address this bug, and would simplify the whole experience.

Comment 5

[Heather Arthur [:harth]]

(In reply to Paul Rouget [:paul] from comment #4)
> (In reply to Heather Arthur [:harth] from comment #3)
> > I can see the Inspector notification being annoying, especially given that
> > you can't save the changes anyways. IMO, we should get rid of the Inspector
> > notification.
> 
> The notification is here mostly to avoid leaving the page by mistake
> (clicking on a link while inspecting for example). So it's needed. But we
> need a way to get rid of it (bug 818754).

Hm, alright. Did someone complain about this before we started showing the notification?

> 
> > For the Style Editor, I'd really want to know if I hadn't
> > saved changes back to disk. I've tried it out with this and I really like
> > having it there.
> 
> Would a global notification system work for you? I think it would address
> this bug, and would simplify the whole experience.

Yep, that would work.

Comment 6

[Heather Arthur [:harth]]

Okay, what do you think: On "will-navigate", toolbox checks all tools for isDirty and throws up a notification saying "If you leave the page, changes made in the Style Editor and Inspector will be lost", listing the tools that will lose changes?

Comment 7

[Paul Rouget [:paul]]

(In reply to Heather Arthur [:harth] from comment #6)
> Okay, what do you think: On "will-navigate", toolbox checks all tools for
> isDirty and throws up a notification saying "If you leave the page, changes
> made in the Style Editor and Inspector will be lost", listing the tools that
> will lose changes?

Something along these lines. This needs to happen on the browser notification box (not the toolbox one), and it needs to provide buttons: "ok, cancel, never ask for this site".

Comment 8

[Heather Arthur [:harth]]

As per bug 818754, we should have a button for disabling this notification for all sites in the future.

At this point I'm wondering if we still want a single notification. I personally would turn off the inspector notification, but leave the Style Editor one. I also don't think there are many times where I use the tools at the same time.

What do you think about having one notification for the Inspector, and one for the Style Editor?

Comment 9

[Joe Walker [:jwalker]]

(In reply to Heather Arthur [:harth] from comment #8)
> What do you think about having one notification for the Inspector, and one
> for the Style Editor?

I think that having a reloadable Style Editor will change how we look at this, so I don't think we should do something long and complex, but if it's simple, maybe separate notifications makes sense.

Comment 10

[Heather Arthur [:harth]]

(In reply to Joe Walker [:jwalker] from comment #9)
> (In reply to Heather Arthur [:harth] from comment #8)
> > What do you think about having one notification for the Inspector, and one
> > for the Style Editor?
> 
> I think that having a reloadable Style Editor will change how we look at
> this, so I don't think we should do something long and complex, but if it's
> simple, maybe separate notifications makes sense.

The Style Editor is an authoring tool and I think it's important we notify the user when they're going to lose their changes. If I'm editing in the Style Editor I almost always intend to save the changes, and leaving the page without saving would be an accident. This isn't the case for the Inspector for me. I would want to turn of the Inspector notifications.

If we say "There are unsaved changes in the Inspector" or show a generic notification when there are inspector changes, and the user opts to disable it, then they have no idea they've just disabled the notifications for the Style Editor as well, and would have a hard time getting it back. </filibuster>

In any case, this bug has a patch that provides the separate Style Editor notification, that we can take out in the next version of Firefox. But, still open to possiblities.

Comment 11

[Paul Rouget [:paul]]

(In reply to Heather Arthur [:harth] from comment #10)
> The Style Editor is an authoring tool and I think it's important we notify
> the user when they're going to lose their changes.

But what if we don't get rid of the changes? But then how to you reset the stylesheets? A button?

What I don't want:
- multiple notifications
- not being able to get rid of notification (even if we only have one)
- being in a situation where the user can lose a lot of his work

What if we just have a notification mechanism for the style editor, and no notification for the inspector? I believe that we are being over cautious with the inspector.

We would take this patch and get rid of the inspector notification.

Comment 12

[Heather Arthur [:harth]]

(In reply to Paul Rouget [:paul] from comment #11)
> (In reply to Heather Arthur [:harth] from comment #10)
> > The Style Editor is an authoring tool and I think it's important we notify
> > the user when they're going to lose their changes.
> 
> But what if we don't get rid of the changes? But then how to you reset the
> stylesheets? A button?

Yeah, I guess I was thinking just a button, but that's for another bug.

> What if we just have a notification mechanism for the style editor, and no
> notification for the inspector? I believe that we are being over cautious
> with the inspector.
> 
> We would take this patch and get rid of the inspector notification.

I would certainly be okay with that.

Comment 13

[Paul Rouget [:paul]]

I'd like to get a second opinion on this (rob?).

reasoning:

2 conflicting facts:
- reloading the page is part of the development/debugging process, we don't want to interrupt the user
- we don't want the user to lose his work

Options we have:
- no notifications and re-apply modifications
  pro: no notifications, can't lose work
  cons: reload is used to reset
- no notifications and we reset modifications
  pro: no notifications, do what is expected (reset)
  cons: lose work
- notification if any tool is "dirty", we reset modifications
  pro: no accidental loss
  cons: annoying
  comment: we can allow the user to disable notifications. But: hard to re-enable, he might not want that for all the tools. So we could have one notification per tool, but then it's getting very annoying.
- notifications only for "big" changes (styleeditor)
  pro: no "big" loss
  cons: no notification small changes (inspector for example)

Me and Heather prefer the latest option.

There might be an alternate way to handle that: latest option + show notification for inspector in case of navigate-away but not reload (which can be done in a follow-up bug)

Comment 14

[Rob Campbell [:rc] (:robcee)]

reading the filibustering in this bug, I agree with the line of thinking you ended up with in comment 12:

1. Removing the notification in the Inspector, and
2. Add a notification to the style editor if changed.

I think Heather's correct asserting that changes to the Style Editor are more important than changes made in the Inspector. I find the notification surprising when I reload the page with changes in the inspector. Style Editor changes feel much more deliberate to me and not something I'm just "Trying out". I don't want to lose them accidentally.

I think this would be an acceptable interim solution along the way to providing the ability to re-apply style changes after a reload, but only if the user asks for them.

I think the workflow of,

1. Making Edits to see how they look locally,
2. applying the edits on the server,
3. reloading the page to view the edits

is a pretty common one and we shouldn't interfere with it unless we're making it better.

I'm mildly against adding a "Never for this site" option as it leads to keeping track of which sites developers are working on and requires them to make a decision every time they make a tweak to a page (for whatever reason) and then try to navigate away.

Comment 15

[Rob Campbell [:rc] (:robcee)]

also, it just occurred to me that at some point, developers are going to be able to make changes in the debugger and we'll need a similar notification mechanism there. Will probably make sense to refactor whatever lands here to a more global mechanism at that time, but I'm fine with just making it work for the Style Editor in the interim.

Comment 16

[Heather Arthur [:harth]]

Attachment: Show notification if unsaved changes - rebased

Rebased patch. Shows a notification if navigating when the Style Editor has unsaved changes.

We can remove the Inspector notification in a separate bug.

Comment 17

[Paul Rouget [:paul]]

Comment on attachment 713540 [details] [diff] [review]
Show notification if unsaved changes - rebased

We need tests.

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 713540 [details] [diff] [review]
Show notification if unsaved changes - rebased

Looks great ... we do need a test though.

Comment 19

[Heather Arthur [:harth]]

Attachment: Show notification if unsaved changes - with test

Patch with requested test. The test is basically a copy of the inspector test that tests the inspector navigation notification.

Comment 20

[Paul Rouget [:paul]]

(In reply to Heather Arthur [:harth] from comment #19)
> Created attachment 719280 [details] [diff] [review]
> Show notification if unsaved changes - with test
> 
> Patch with requested test. The test is basically a copy of the inspector
> test that tests the inspector navigation notification.

Does this need a review?

Comment 21

[Heather Arthur [:harth]]

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

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/315a1bf40723

Comment 23

[Jan Tojnar]

What happened with the Style Editor unsaved changes warning? Was it removed with Inspector warning (https://bugzilla.mozilla.org/show_bug.cgi?id=850513)? Yesterday I lost an hour of work by accidentally clicking a link on a page whose style I was editing.