1476366 - Track Changes - Prototype client-side implementation for style changes

Status: RESOLVED WONTFIX

(DevTools :: Inspector, enhancement, P2)

Description

[Razvan Caliman [:rcaliman]]

This bug is just a host for code and conversations about a prototype implementation for the Track Changes feature happening in Bug 1429247.

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1476366 - (Part 1) Instrument Rule and TextProperty models to emit event on change.

Review commit: https://reviewboard.mozilla.org/r/257572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257572/

Comment 2

[Razvan Caliman [:rcaliman]]

Comment on attachment 8992717 [details]
Bug 1476366 - (Part 1) Instrument Rule and TextProperty models to emit event on change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257572/diff/1-2/

Comment 3

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1476366 - (Part 2) Track and serialize style changes; Setup Redux store + React-based changes panel.

Review commit: https://reviewboard.mozilla.org/r/258156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258156/

Comment 4

[Razvan Caliman [:rcaliman]]

Comment on attachment 8992717 [details]
Bug 1476366 - (Part 1) Instrument Rule and TextProperty models to emit event on change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257572/diff/2-3/

Comment 5

[Brad Werth [:bradwerth]]

This is my first exposure to Redux. I believe I am following the logic, and the code seems to me to be well-structured. Some thoughts:

1) In the track function, for inline stylesheets, we would need to capture the DOM node. Is that the "tag" property?
2) The track function seems to rely on this.inspector.selection to get more information about the rule. Is there a way this function could get called where it wasn't tied to the current selection?
3) Under what conditions is rule.isInlineStyle true such that the earlier logic in the track function is not sufficient to characterize the change?
4) The logic in updateDiff to coalesce remove-after-add changes... I think we should expand the cases where we coalesce changes depending on the behavior of the rules view UI. For example, if the user typing a partial value triggers a preview of the value, that would be a change, I think. And then if the user continues typing, that would trigger another change. In cases like that, I think we'd better serve the user by coalescing that into a single change for an eventual undo/redo stack. I think we'll have several cases like this and we might want to break the logic into its own function (outside of updateDiff) so we can make clear what all the cases are and why they are justified.

This is a great prototype. Lots of promise here. Thank you for posting it.

Comment 6

[Razvan Caliman [:rcaliman]]

> 1) In the track function, for inline stylesheets, we would need to capture the DOM node. Is that the "tag" property?
No. The tag property is the tag name of the matched node. It's not used yet, but I intend to show it in the "inline styles" group of changes as element + generated unique selector (just the generated selector shows up now and it may be confusing).

It is possible to identify the exact inline stylesheet. It's just not implemented in the prototype yet.

> 2) The track function seems to rely on this.inspector.selection to get more information about the rule. Is there a way this function could get called where it wasn't tied to the current selection?

`this.inspector.selection` is just used to get the NodeFront of the currently selected element and use that to get the tagname and generated unique selector (it's a method that lives on the Node Actor). It is not necessary for any other changes (inline stylesheets & external stylesheets). With appropriate guards, this method could be called without requiring `this.inspector.selection` to exist (e.g.: called from Style Editor).

> 3) Under what conditions is rule.isInlineStyle true such that the earlier logic in the track function is not sufficient to characterize the change?

`rule.isInlineStyle` is an explicit check if the rule is actually modeled from an *inline style attribute*. The previous checks assumes it's an *inline stylesheet* if the href property of the source stylesheet is null. For inline styles attributes, the source stylesheet href is also null, hence the need to explicitly check for rule.isInlineStyle.

> 4) if the user typing a partial value triggers a preview of the value, that would be a change, I think. And then if the user continues typing, that would trigger another change. In cases like that, I think we'd better serve the user by coalescing that into a single change for an eventual undo/redo stack.

This is the current behaviour, if I'm not misunderstanding your example. Only commits are tracked, not previews. 
For example, changing the color with the color picker will commit and track a change once the color picker is closed, not as the used is exploring colors. I believe this is the right approach here in order to not pollute undo history with needless (read: uncommitted) changes. 

For other free-form tools, like the Shape Path Editor, CSS Filter Editor or the Font Editor, there is a means for high-volume changes (dragging sliders) but no explicit commit action. For such cases, I think we need to implement a sensible debounce (log change after not being called for X milliseconds) or throttling (log change every X milliseconds).

For undo/redo, I propose serializing objects with mutations for "forward" and "backward". These may be derived from the TRACK_CHANGE reducer. Additional data is missing from the change object in the current prototype, like Media Query text or Keyframe rule name. But this information exists on the Rule model and we can access it at "change" time. This is an advantage of the client-side implementation.

Comment 7

[Razvan Caliman [:rcaliman]]

This bug's patches have been cleaned-up and repurposed in Bug 1478448 and Bug 1478612. Closing this now.