Closed Bug 1557689 Opened 6 months ago Closed 4 months ago

[Inactive CSS] Properties sometimes not marked as inactive when changing condition in another rule

Categories

(DevTools :: Inspector: Rules, defect, P1)

defect

Tracking

(firefox70+ verified)

VERIFIED FIXED
Firefox 70
Tracking Status
firefox70 + verified

People

(Reporter: rcaliman, Assigned: rcaliman)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

When more than one CSS rule applies and the property used as a condition for checking inactive CSS is toggled, the properties which become invalid in other matching rules do not get marked as invalid.

Steps to reproduce

  • Run this in the address bar of a new tab:
data:text/html,<style>div { display:flex } .end{ justify-content: end }</style><div class="end">TEST
  • Open DevTools and inspect the <div> element
  • Disable the display: flex declaration

Expected

  • The justify-content: end property should be marked as invalid.

Actual

  • The justify-content: end property still shows as valid.

Workaround
Select another element, then select the <div> again to see the accurate list of valid and invalid properties.

Duplicate of this bug: 1565244
Summary: [Inactive CSS] Properties not marked as invalid when changing the condition in a different CSS rule → [Inactive CSS] Properties sometimes not marked as inactive when changing condition in another rule

Take this CSS

.end {
  justify-content: end;
}

div {
  display: flex;
}

This is what happens:

Summary

We immediately update the UI

  1. The checkbox next to display:flex is clicked, disabling the property.
  2. We update the editor (crossing out and re-evaluating isUsed for display:flex).
  3. The client emits property-value-updated.
  4. The server emits track-change along with the changes.
  5. The track-change data is stored.
  6. Changes are queued for the div rule along with display:flex.

We apply the changes to the stylesheets

  1. stylesheets::update() is called to update the style sheet in place with new text.
  2. A style-applied event is triggered causing the styles::_styleApplied() method to reset cssLogic.
  3. The style-applied event is triggered.
  4. styles::_onStyleApplied() is called for the div rule.
  5. styles::_onStyleApplied() is called for the .end rule.
  6. stylesheets::update() sends a media-rules-changed event.
  7. The rule URL is updated.
  8. A media-rules-changed (I don't think this affects the rule view).
  9. [styles::form] is triggered recalculating isUsed for div.
  10. onRuleUpdated() is called.
  11. We call element-style::updateDeclarations().
  12. We try to update the property state but WRONGLY believe that justify-content::used = true because the style change has not yet been applied to the content.

We fail to respond to the reflows

  1. At this point we realize that something changed in the rule view.
  2. The reflow actor also notices these changes and sends a reflows event.
  3. The reflows actor tells us which node has changed.
  4. A second reflows event.

The problem

  1. WE SHOULD UPDATE THE UI HERE BUT THIS DOES NOT HAPPEN.

Comments

When viewed in summary like this the flow makes sense but for the first time InactiveCSS added logic that needs to be live i.e. can't simply be updated when there is a change in the UI.

The larger problem here is that adding more complexity here will make this code even more unmaintainable but I don't believe we have a choice.

Detail

We immediately update the UI (Detail)

  1. The checkbox next to display:flex is clicked, disabling the property:
    CLIENT [text-property::setEnabled()]
    CLIENT [rule::setPropertyEnabled]

  2. We update the editor (crossing out and re-evaluating isUsed for display:flex):
    CLIENT [text-property::updateEditor()]
    CLIENT [text-property-editor::update]
    CLIENT [text-property-editor::updatePropertyState]
    CLIENT [text-property:isUsed] display::used = true
    CLIENT [text-property-editor::updatePropertyState] display::used = true

  3. The client emits property-value-updated:
    EMITTING: emit(property-value-updated, {rule: [object Object], property: display, value: flex}) from update@resource://devtools/client/inspector/rules/views/text-property-editor.js:516:19

  4. The server emits track-change along with the changes:
    EMITTING: emit(track-change, {id: server0.conn0.child1/domstylerule50, ancestors: , selector: div, source: [object Object], ruleIndex: 0, type: declaration-disable, add: null, remove: [object Object]}) from trackChange@resource://devtools/server/actors/utils/track-change-emitter.js:22:10

  5. The track-change data is stored
    ?????? [event-emitter::bound pushChange] (
    {},
    {id: server0.conn0.child1/domstylerule50, ancestors: , selector: div, source: [object Object], ruleIndex: 0, type: declaration-disable, add: null, remove: [object Object]},
    )
    SERVER [string::initialize] (
    {},
    {_prefix: server0.conn0.child1/, _transport: [object Object], _nextID: 58, _socketListener: null, _actorPool: [object Object], _extraPools: [object Object],[object Object],[object Object],[Actor frame..., _actorResponses: [object Map], _forwardingPrefixes: [object Map], parentMessageManager: [object ContentFrameMessageManager], currentPacket: undefined},
    )

  6. Changes are queued for the div rule along with display:flex:
    ?????? [lexer::map] (
    div,
    function anonymous(),
    )
    ?????? [lexer::map] (
    display,
    function anonymous(),
    )
    ?????? [lexer::map] (
    flex,
    function anonymous(),
    )

We apply the changes to the stylesheets (Detail)

  1. stylesheets::update() is called to update the style sheet in place with new text:
    SERVER [stylesheets::update]
    SERVER [stylesheets::update] sending style-applied event
    EMITTING: emit(style-applied, 0, {actorID: server0.conn0.child1/stylesheet40, _actorSpec: [object Object], rawSheet: [object CSSStyleSheet], parentActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], conn: [object Object], text: div { /_! display:flex / } .end{ justify-content: end }, _styleSheetIndex: 0, ownerDocument: [object HTMLDocument]}) from update@resource://devtools/server/actors/stylesheets.js:583:12
    ?????? [event-emitter::anonymous] (
    {actorID: server0.conn0.child1/stylesheet40, _actorSpec: [object Object], rawSheet: [object CSSStyleSheet], parentActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], conn: [object Object], text: div { /
    ! display:flex / } .end{ justify-content: end }, _styleSheetIndex: 0, ownerDocument: [object HTMLDocument]},
    0,
    {actorID: server0.conn0.child1/stylesheet40, _actorSpec: [object Object], rawSheet: [object CSSStyleSheet], parentActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], conn: [object Object], text: div { /
    ! display:flex _/ } .end{ justify-content: end }, _styleSheetIndex: 0, ownerDocument: [object HTMLDocument]},
    )

  2. A style-applied event is triggered causing the styles::_styleApplied() method to reset cssLogic:
    ?????? [event-emitter::bound styleApplied] (
    {actorID: server0.conn0.child1/stylesheet40, _actorSpec: [object Object], rawSheet: [object CSSStyleSheet], parentActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], conn: [object Object], text: div { /
    ! display:flex / } .end{ justify-content: end }, _styleSheetIndex: 0, ownerDocument: [object HTMLDocument]},
    0,
    {actorID: server0.conn0.child1/stylesheet40, _actorSpec: [object Object], rawSheet: [object CSSStyleSheet], parentActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], conn: [object Object], text: div { /
    ! display:flex _/ } .end{ justify-content: end }, _styleSheetIndex: 0, ownerDocument: [object HTMLDocument]},
    )
    SERVER [styles::_styleApplied]
    SERVER [styles::_styleApplied] calling cssLogic.reset()

  3. The style-applied event is triggered:
    ?????? [event-emitter::bound onStyleApplied] (
    {actorID: server0.conn0.child1/stylesheet40, _actorSpec: [object Object], rawSheet: [object CSSStyleSheet], parentActor: [Actor frameTarget/server0.conn0.child1/frameTarget1
    EMITTING: emit(style-applied, 0, {conn: [object Object], __poolMap: null, actorID: server0.conn0.child1/stylesheet40, _requests: , _frontListeners: [object Object], _beforeListeners: [object Map], _onPropertyChange: function() {
    [native code]
    }, _form: [object Object]}) from onPacket@resource://devtools/shared/protocol/Front.js:200:13
    ], conn: [object Object], text: div { /
    ! display:flex / } .end{ justify-content: end }, _styleSheetIndex: 0, ownerDocument: [object HTMLDocument]},
    0,
    {actorID: server0.conn0.child1/stylesheet40, _actorSpec: [object Object], rawSheet: [object CSSStyleSheet], parentActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], conn: [object Object], text: div { /
    ! display:flex _/ } .end{ justify-content: end }, _styleSheetIndex: 0, ownerDocument: [object HTMLDocument]},
    )

  4. styles::_onStyleApplied() is called for the div rule.
    SERVER [styles::_onStyleApplied] this._ruleIndex = 1
    EMITTING: emit(location-changed, 1, 29) from _notifyLocationChanged@resource://devtools/server/ actors/styles.js:1565:10
    ?????? [event-emitter::anonymous] (
    {actorID: server0.conn0.child1/domstylerule49, _actorSpec: [object Object], pageStyle: [Actor pagestyle/server0.conn0.child1/pagestyle24], rawStyle: [object CSS2Properties], _parentSheet: [object CSSStyleSheet], _onStyleApplied: function() {
    [native code]
    }, _declarations: [object Object], type: 1, rawRule: [object CSSStyleRule], _ruleIndex: 1},
    1,
    29,
    )
    ?????? [event-emitter::bound onStyleApplied] (
    {actorID: server0.conn0.child1/stylesheet40, _actorSpec: [object Object], rawSheet: [object CSSStyleSheet], parentActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], conn: [object Object], text: div { /
    ! display:flex / } .end{ justify-content: end }, _styleSheetIndex: 0, ownerDocument: [object HTMLDocument]},
    0,
    {actorID: server0.conn0.child1/stylesheet40, _actorSpec: [object Object], rawSheet: [object CSSStyleSheet], parentActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], conn: [object Object], text: div { /
    ! display:flex _/ } .end{ justify-content: end }, _styleSheetIndex: 0, ownerDocument: [object HTMLDocument]},
    )

  5. styles::_onStyleApplied() is called for the .end rule.
    SERVER [styles::_onStyleApplied] this._ruleIndex = 0

  6. stylesheets::update() sends a media-rules-changed event:
    SERVER [stylesheets::update] sending media-rules-changed event
    EMITTING: emit(media-rules-changed, []) from update/<@resource://devtools/server/actors stylesheets.js:590:12
    EMITTING: emit(location-changed, 1, 29) from onPacket@resource://devtools/shared/protocol Front.js:200:13
    ?????? [event-emitter::anonymous] (
    {actorID: server0.conn0.child1/stylesheet40, _actorSpec: [object Object], rawSheet: [object CSSStyleSheet], parentActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], conn: [object Object], text: div { /_! display:flex _/ } .end{ justify-content: end }, _styleSheetIndex: 0, ownerDocument: [object HTMLDocument]},
    [],
    )

  7. The rule URL is updated:
    EMITTING: emit(source-link-updated) from updateSourceLink/<@resource://devtools/client/inspector/ rules/views/rule-editor.js:429:12
    ?????? [lexer::map] (
    display,
    function anonymous(),
    )
    ?????? [lexer::map] (
    flex,
    function anonymous(),
    )

  8. A media-rules-changed (I don't think this affects the rule view):

    EMITTING: emit(media-rules-changed, []) from onPacket@resource://devtools/shared/protocol/Front.js:200:13

  9. [styles::form] is triggered recalculating isUsed for div:
    SERVER [styles::form] Recalculating isUsed for div
    SERVER [styles::form] display::used = true

  10. onRuleUpdated() is called:
    CLIENT [rule::applyProperties] Calling onRuleUpdated()
    CLIENT [element-style::onRuleUpdated]

  11. We call element-style::updateDeclarations():
    CLIENT [element-style::updateDeclarations]

  12. We try to update the property state but WRONGLY believe that justify-content::used = true because the style change has not yet been applied to the content:
    CLIENT [text-property-editor::updatePropertyState]
    CLIENT [text-property:isUsed] justify-content::used = true
    CLIENT [text-property-editor::updatePropertyState] justify-content::used = true

We fail to respond to the reflows (Details)

  1. At this point we send a ruleview-changed event:
    EMITTING: emit(ruleview-changed) from _changed@resource://devtools/client/inspector/rules/rules.js:1127:10

  2. The reflow actor also notices these changes and sends a reflows event:
    EMITTING: emit(reflows, [{start:8204.813903, end:8204.941783, isInterruptible:true}]) from _startEventLoop@resource://devtools/server/actors/reflow.js:315:12
    ?????? [event-emitter::bound _onReflows] (
    {targetActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], _startEventLoop: function() {
    [native code]
    }, _onReflow: function() {
    [native code]
    }, _onResize: function() {
    [native code]
    }, reflowObserver: [object Object], resizeObserver: [object Object], isObserving: true, reflows: [object Object], hasResized: false, eventLoopTimer: 1155},
    [{start:8204.813903, end:8204.941783, isInterruptible:true}],
    )

  3. The reflows actor tells us which node has changed:
    EMITTING: emit(display-change, ) from _onReflows@resource://devtools/server/actors/inspector walker.js:591:12
    ?????? [event-emitter::anonymous]

  4. A second reflows event:
    ?????? [event-emitter::bound _onReflow] (
    {targetActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], _startEventLoop: function() {
    [native code]
    }, _onReflow: function() {
    [native code]
    }, _onResize: function() {
    [native code]
    }, reflowObserver: [object Object], resizeObserver: [object Object], isObserving: true, reflows: [object Object], hasResized: false, eventLoopTimer: 1155},
    [{start:8204.813903, end:8204.941783, isInterruptible:true}],
    )
    EMITTING: emit(reflows, [{start:8204.813903, end:8204.941783, isInterruptible:true}]) from _onReflow@resource://devtools/server/actors/reflow.js:81:12
    ?????? [event-emitter::anonymous] (
    {conn: [object Object], actorID: server0.conn0.child1/reflowActor8, _actorSpec: [object Object], targetActor: [Actor frameTarget/server0.conn0.child1/frameTarget1], _onReflow: function() {
    [native code]
    }, observer: [object Object], _isStarted: true, _pendingResponse: [object Promise]},
    [{start:8204.813903, end:8204.941783, isInterruptible:true}],
    )

The problem (Details)

  1. WE SHOULD UPDATE THE UI HERE BUT THIS DOES NOT HAPPEN.

@Razvan @gl So the obvious thing to do here is call stylesheets::update() after the form updates isUsed when updating the declarations but that causes an infinite loop and breaks the world.

Do either of you see another way around this?

Flags: needinfo?(razvan.caliman)
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED

Unfortunately, I don't have any solutions to your problem. I will add a couple of notes based on my observation which might help feed this discussion. I haven't gone too deep in testing this, so, my observations might just be invalid.

We don't currently use or respond to any reflows event in the Rules view. The closest thing we have is InspectorStyleChangeTracker, which only listens to resize events from the walker.

I would like us to explore options where we ensure that by step 11 where we call element-style::updateDeclarations() that the style change has already been applied to the content. That would remove additional listeners or another roundtrip to update.

Another thought that comes to my mind is that isUsed is static since it's in the form. I am not quite sure this is a good idea since you would have to explicitly overwrite it in order for it to be updated. I have seen this with the walker and how it queues node mutations to update the node's form() https://searchfox.org/mozilla-central/source/devtools/shared/fronts/node.js#189. So, is it possible that we are just always returning the same value for isUsed because it is in the form?

Flags: needinfo?(gl)

(In reply to Gabriel [:gl] (ΦωΦ) from comment #4)

Unfortunately, I don't have any solutions to your problem. I will add a couple of notes based on my observation which might help feed this discussion. I haven't gone too deep in testing this, so, my observations might just be invalid.

Anything is welcome at this point.

We don't currently use or respond to any reflows event in the Rules view. The closest thing we have is InspectorStyleChangeTracker, which only listens to resize events from the walker.

Yes, I left them in just to give a complete picture.

I would like us to explore options where we ensure that by step 11 where we call element-style::updateDeclarations() that the style change has already been applied to the content. That would remove additional listeners or another roundtrip to update.

The irony is that before step 11 the styles have not been applied to content yet... our calculations are correct but they are based on a DOM that is lagging behind.

Maybe we could use requestIdleCallback before updating the UI.

Another thought that comes to my mind is that isUsed is static since it's in the form. I am not quite sure this is a good idea since you would have to explicitly overwrite it in order for it to be updated. I have seen this with the walker and how it queues node mutations to update the node's form() https://searchfox.org/mozilla-central/source/devtools/shared/fronts/node.js#189. So, is it possible that we are just always returning the same value for isUsed because it is in the form?

isUsed being static is the root of the problem but we are having trouble finding anywhere else that we could put this code. We don't necessarily always return the same value but it may not be up-to-date when we update the UI.

Tracking this for 70 since it blocks the feature.

{F1513187}

This is a prototype implementation to fix the bug whereby changing a declaration in one rule will not update the inactive flag of declarations in another rule. It's a hack. It works but it has caveats. Read on.

##Test plan:

  • Run this in the address bar of a new tab:
data:text/html,<style>body {display: flex} .test{justify-content:center}</style><body class="test">
  • Open the DevTools Inspector, switch to Rules view
  • Disable the display: flex declaration.

The justify-content: center declaration should update to show it is now unused.

How it works

After making any change to a CSS declaration, the ElementStyle model makes a request to a new method on the PageStyleActor to ask for the updated state of all declarations in all rules matching the current element. Iterating through rules and assuming the number and order of declarations hasn't changed, it uses the fresh declaration data to then replace the isUsed() method on the TextProperty (the model representing a CSS declaration) to point to the updated used flag state.

The TextPropertyEditor is the view for the TextProperty. After mutating the text property, call the existing TextPropertyEditor.updatePropertyState() method to reflect the used flag state in the Rule view. This is a cheaper method than TextPropertyEditor.update().

The main issue to circumvent here is that declarations on the StyleRuleFront are fixed in time because they're exposed via a call to form() which locks them to a state at the front creation time. Ideally, the declarations should be exposed directly on the StyleRuleFront so repeated requests for them would yield fresh data. That would require a backwards-incompatible change of the interface exposed via protocol.js. But that may be justified if we get additional value from the change plus a reduction in the complexity of the CSS editing workflow.

I intuit that this prototype is making editing slower than before. It doesn't feel like it, but it certainly does additional work server-side to recompute the isUsed flag on all CSS declarations of all rules matching the element. Client-side, all declarations' state is refreshed without any checks (this can perhaps be guarded better). Any performance degradation would be proportional to the number of rules and declarations matched. I ran a DAMP run to see if we can observe the perf degradation with our existing tests.

The pros of this approach:

  • it seems to work and solves the issue

The cons of this approach is that:

  • it adds extra complexity to the code for a very specific and narrow use case
  • it cycles through all rules and all declarations (this could perhaps be better guarded with property/value checks at the expense of even more added complexity)
  • it adds yet more indirection in the CSS editing workflow (models poking into and calling other models' views)
  • it degrades performance (slightly or more pronounced; to be seen)

Another idea

This prototype uses a "pull" approach, whereby the ElementStyle explicitly asks for updates and demands Rules update their text properties.

Some of this indirection may be mitigated by a "push" approach, whereby we add a new event on the StyleRuleFront to signal that a rule's declarations have changed. There is precedent with the "location-changed" event.

One approach may be to store the latest rules returned by StyleRuleActor.getApplied() in a temporary array as an indicator that they're being used by a consumer (ex: Rules view via the ElementStyle model). When applying a CSS change server side, this list of rules could be iterated through and have their declarations' state refreshed (ex: isUsed flag). If there is a change in state, the rule would fire a "declarations-updated" event with either all or just the changed declarations and let the client reconcile the changes.

The benefits of this approach:

  • only the rules which have actually change trigger changes on the client.
  • co-locates logic about changed state on the server
  • the consumer doesn't explicitly have to ask for updates in order to receive them (for example if other tools mutate the CSS rule, like Font Editor)

The potential downsides:

  • having a succession of events, one for each rule, going over the protocol.js wire instead of just one big object
Flags: needinfo?(razvan.caliman)

This is an alternative implementation to D41996.
The main difference is that it's now a "push" approach whereby changes to one CSS rule will refresh the CSS declarations of all rules matching the element. If any declaration's used state changed, an event, "declarations-changed", is triggered by the StyleRuleActor. On the client, consumers go through the declarations with changed "isUsed" flags and update the UI accordingly.

Some optimizations are in place to not check declarations that don't have Inactive CSS validators and to fire the "declarations-changed" only for rules which have declarations which have actually changed the "isUsed" flag.

TODO: we should rename declaration.isUsed or make it a boolean and tack the other metadata (fixId, msgId, etc) onto another exposed object on the declaration. It's confusing to have it return an object given its name.

Attachment #9085524 - Attachment is obsolete: true
Attachment #9085761 - Attachment description: Bug 1557689 - Refresh all CSS rules matching an element after making changes to one → Bug 1557689 - Refresh all CSS rules matching an element after making changes to one. r=gl

Re-assigning to Razvan as he's the one working on putting in place the fix.

Assignee: mratcliffe → rcaliman
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b79001cbab82
Refresh all CSS rules matching an element after making changes to one. r=gl
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

Verified with 70.0a1 (2019-08-28) .

Status: RESOLVED → VERIFIED
Regressions: 1593944
No longer regressions: 1593944
You need to log in before you can comment on or make changes to this bug.