Track Changes - Track CSS changes from markup view



6 months ago
4 months ago


(Reporter: rcaliman, Unassigned)


(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)



(1 attachment)



6 months ago
Track CSS changes done to inline styles in the markup view:
- add `style` attribute
- edit `style` attribute value
- remove `style` attribute
Relevant here: there are probably 2 ways to track this: either by hooking into the markupview's attribute edition machinery (or my listening to the markupmutation event), or by hooking into the rule-view's inline style edition.
In any case, we'll need to avoid listening from both places to avoid tracking things twice (e.g. if the user changes the inline style in the markup-view, that will change the style attribute too)
Severity: normal → enhancement


6 months ago
Blocks: track-changes
No longer blocks: 1503920
I've been thinking a little bit about this today, and came up with the following code change that, I think, does something right. I'm not sure it's totally correct, but I think the entry point is probably the right one, and removing all declarations and re-adding all new declarations feels like the right logic to me.
Razvan, does this help?
Flags: needinfo?(rcaliman)
While doing this, I realized that the expected structure for a change object isn't documented anywhere. So I had to reverse-engineer it by looking at the logDeclarationChange function in the StyleRuleActor.
I think we need to document it in the Changes actor, and perhaps even validate it there (i.e. throw errors if the supplied change object isn't correct).

Comment 4

6 months ago
Thank you for investigating this, Patrick! This is great stuff!

There are some limitations to this method, but it does work in the common use cases. It makes the feature usable and that's important. If you'd like to work on this for the imminent 65 release, I'd be happy to review it. If that's too short notice, no problem. It can be done afterwards and perhaps uplifted too. I don't have time to actively work on it this week.

One bug:
If either the new or old attribute value is null, the parseNamedDeclarations() method throws. It just needs a few guards. 

Some cases with unexpected results:

These are not the fault of this code. They're mostly limitations on the current Track Changes architecture. Mainly, the index of the tracked CSS declaration can collide due to the changing structure of the declaration block being changed (the contents of the style attribute). 

1) Example:
- Run this in a new tab:
data:text/html,<div style="color:red; background:white">TEST

- Delete the "color:red" declaration from the style attribute in the markup view.

Result: In the Changes panel, the unchanged "background:white" declaration shows up as removed *and* added. This happens because the declaration itself changed position in the rule. It's tracked once at index 1 and then at index 0 (because it's the only one left). The reconciliation logic doesn't apply because it considers these as two different declarations. This problem affects other cases which are more difficult to get into, but still possible, in the Rules view.

2) Example:
- Run this in a new tab:
data:text/html,<div style="color:red; background:white">TEST

- Disable "background:white" in the Rules view
- Edit the style attribute and remove the "/*! background:white */" string

Result: In the Changes panel, "background:white" shows as removed twice and "color:red" also shows up as removed and added back. This too is an issue with indices, but primarily caused by the fact that we track changes incrementally instead of from the very first state. Over time, the "previous" state changes, but what was tracked in the Changes panel is correlated with the "previous" state at the time of logging so properties don't cancel out because they don't match up anymore.

The solution for this is beyond this patch. We need to make Track Changes diff against the first state of the styles, not against the intermediary states.

3) Example:
- Run this in a new tab:
data:text/html,<div style="color:red;">TEST

- Edit the markup view and add an incomplete declaration "background:"

Result: In the Changes panel, the incomplete declaration is tracked. This case is handled by the Rules view, because it immediately issues a "remove" operation to cancel it out, but the Markup view does not. 

This case also should be handled higher in the Track Changes logic (i.e.: do not track value-less declarations), but it does trip other logic in the workflow to add a declaration via the Rules view.

4) Another potential issue that needs to be solved upstream is introducing transactions for changes -  the ability to log multiple declaration changes in one operation. Right now, each existing declaration is first tracked as removed, then added back in. If they didn't change, the operations cancel each other out and don't show up in the Changes panel. That's correct. But they will get logged in the undo/redo queue which can confuse users. The undo/redo behavior not yet implemented, so it doesn't create a problem yet, but if we go this route, we need to log a bug to remind ourselves of the nuance.
Flags: needinfo?(rcaliman)


4 months ago
Component: Inspector → Inspector: Changes
You need to log in before you can comment on or make changes to this bug.