1500979 - Track Changes - warn the user when a stylesheet is replaced

Status: NEW

(DevTools :: Inspector: Changes, enhancement, P2)

Description

[Razvan Caliman [:rcaliman]]

For M1 scope, we don't track changes happening via the Style Editor because it just replaces the stylesheet text with the updated stylesheet. This behaviour (replacing a stylesheet completely) also happens with hot-reloading and can cause previously tracked changes to become invalid. 

Reconciling between old and new stylesheets can be complex. We will not tackle it in M1 scope. However, as a fist step, we want to mitigate this issue by communicating as clear as possible to the user what's happening and why we're about to lose their tracked changes.

The Changes panel should warn the user when a full stylesheet swap happens and give them a chance to copy/export their changes before resetting the panel.

Comment 1

[Razvan Caliman [:rcaliman]]

For hot-reloading, one way to approach this is to add a listener in the Changes actor for the "stylesheet-added" event fired by the target actor.

In the handler, we check the StyleSheetActor's href against any of the source hrefs already collected in the this.changes array in the Changes actor.
If there's a match, it means one of the stylesheets we've previously collected changes for has been reloaded (perhaps by a hot reloading script). We should then fire an event from the Changes actor to notify the client, devtools/client/inspector/changes/ChangesView.js.

On the client we can listen to this new event and show a warning that the collected changes may no longer be accurate due to the stylesheet reload and prompt the user to copy their changes before continuing (at their own risk) or refreshing the page.

For covering the Style Editor use case (the whole stylesheet content gets replaced when the user edits), we need to react to the "style-applied" event on the StyleSheetActor when the kind argument is UPDATE_GENERAL.

Comment 2

[v.yathan]

hey can this be assigned to me? started working on it.

Comment 3

[Razvan Caliman [:rcaliman]]

Hi v.yathan,

This isn't really a good-first-bug to start working on if you don't have experience with DevTools.

I can assign it to you, but be warned, you'll be going deep into the code and have to understand the server-side Actor structure and Actor<-->Front communication flow. Are you really sure you want to start with this?

Comment 4

[v.yathan]

Hi rcaliman,

we are actually doing this as part of our course CSCD01 at University of Toronto Scarborough, with a group of 4.

we have worked on a couple bugs already, just didn't get them all processed to get merged in. (the actual pull request doesn't affect our marks, just the completion of the fix on our side)

so if its alright I would like it assigned to me, and I can let you know, if we have any problems with it.

Thanks,
Yathan Vidyananthan

Comment 5

[Razvan Caliman [:rcaliman]]

Ok Yathan,

I will assign this to you. Aside from the explanations in comment #1, the DevTools documentation about the backend and clinet-server communication protocol will be useful.

Let me know when you need guidance.

Comment 6

[v.yathan]

We have gotten the changes.js listener to emit and event to changesview.js but the changesview.js file is not picking up the event. We are using this.emit("changed-sheet"); to emit the event within changes.js and listening for it in changesview.js but it doesn't seem to be picking it up. Any ideas why?

Comment 7

[Razvan Caliman [:rcaliman]]

Hi Yathan,

The server (Actor) communicates to the client (Front) via a predefined protocol. If you introduce new events or methods on the server that need to be exposed to the front, you will also need to update the corresponding Spec.

In your case, you have to define the new "changed-sheet" event in https://searchfox.org/mozilla-central/source/devtools/shared/specs/changes.js

An in-depth example is in the "Writing Actors With protocol.js" chapter of the docs.

Comment 8

[Razvan Caliman [:rcaliman]]

Hi, how are you doing on this feature? Do you need help continuing?

Comment 9

[Razvan Caliman [:rcaliman]]

No answer from contributor for 1 month, unassigning this bug.