1771113 - Removing <style>/<link> node doesn't remove stylesheet in StyleEditor

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

There are no events sent from the platform when a stylesheet is removed at the moment, so we should start with that.
Then it should be a matter of firing onResourceDestroyed for stylesheet resources and having a dedicated method in the StyleEditorUI.jsm

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1771113 - Add chromeOnly `StyleSheetRemoved` event, emitted when a stylesheet is removed. r=emilio.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Emilio, as discussed yesterday, I call PostStyleSheetApplicableStateChangeEvent from DocumentOrShadowRoot::RemoveStyleSheet (see D147271)
It does work in the end, but if feels a bit off.
The stylesheet is still seen as applicable, so that's not something we can discriminate on; I'm just looking if the stylesheet was registered and if it doesn't have an associated document anymore, which is okay-ish I guess (unless I'm missing something)
But really, the applicable state for the stylesheet didn't "changed".

So should we have another event for this? Or should we make StyleSheet.h check if we have an associated document?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Right... So, I'd like for the "applicable" state (at least internally in Gecko) to be just "is complete and not disabled" (which is what it does now). It's perfectly legit for non-document-associated sheets (like user or user-agent sheets) to be applicable.

Another event might be sensible, yeah, I guess "applicable state" is not the only thing that devtools cares about. Then again it seems a bit overkill to have a new event just for this. But it might be the cleaner over-all.

Comment 4

[Sebastian Zartner [:sebo]]

If I understand Emilio correctly, this bug covers any kind of removal from the document, i.e. also disabling a stylesheet or removing it from Document.adoptedStyleSheets, right?

I am asking because I just saw a related question on Stack Overflow and I was searching whether there's already a bug filed for it.

Sebastian

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Well changing disabled would cause the sheet not to be applicable, so the patch above would work, yeah.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1771113 - Fix and adapt events and tests. r=nchevobbe

Make sure that when removed the applicable state of the style sheet is
false rather than true. Fix tests too (there were a couple typos etc).

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1771113 - [devtools] Notify about removed stylesheet resources. r=#devtools-reviewers.

Depends on D147271

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1771113 - [devtools] Remove stylesheets from StyleEditor on STYLESHEET resource destroyed. r=#devtools-reviewers.

Depends on D185790

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98df24408c27
Add chromeOnly `StyleSheetRemoved` event, emitted when a stylesheet is removed. r=emilio.
https://hg.mozilla.org/integration/autoland/rev/a8a1e12c259c
[devtools] Notify about removed stylesheet resources. r=devtools-reviewers,ochameau.
https://hg.mozilla.org/integration/autoland/rev/94d6dd1430fc
[devtools] Remove stylesheets from StyleEditor on STYLESHEET resource destroyed. r=devtools-reviewers,ochameau.

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/98df24408c27
https://hg.mozilla.org/mozilla-central/rev/a8a1e12c259c
https://hg.mozilla.org/mozilla-central/rev/94d6dd1430fc