[webkit-changes] [WebKit/WebKit] 0d990d: Web Inspector: Adding a new style property to a ru...

[Patrick Angle]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 0d990da3c01519907affff83cfdee71283f2ef4f
      https://github.com/WebKit/WebKit/commit/0d990da3c01519907affff83cfdee71283f2ef4f
  Author: Patrick Angle <pangle at apple.com>
  Date:   2022-11-07 (Mon, 07 Nov 2022)

  Changed paths:
    M LayoutTests/inspector/css/add-css-property-expected.txt
    M LayoutTests/inspector/css/add-css-property.html
    M Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
    M Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js

  Log Message:
  -----------
  Web Inspector: Adding a new style property to a rule after a commented out property results in incorrect style text/bad frontend state
https://bugs.webkit.org/show_bug.cgi?id=247139
rdar://88824802

Reviewed by Devin Rousso.

Two problems existed that together caused us to incorrectly associate incoming edits to properties following a
commented-out property with that commented out property, or to create a new WI.CSSProperty instead of using the existing
one that is being edited, resulting in various garbage text being written to the style sheet.

The first issue is in how we handle `pendingProperties` in WI.CSSStyleDeclaration. We currently consider a pending
property to be one that is not enabled (e.g. commented out), which does not match the intent of the comments in the
code and nor does it match its usage. The intended use for `pendingProperties` is to house properties that are no
longer part of the CSSStyleDeclaration after an update, presumedly to allow us to reuse `WI.CSSProperty`s during
undo/redo operations. It is also imaginable that this is where properties being edited should briefly exist if after
their creation, we receive an update from the backend before we have received the update that commits the property under
edit.

The misclassification of what belongs in `pendingProperties` is was not enough by itself to cause the issue however. The
issues arose when DOMNodeStyles._parseStylePropertyPayload attempted to associate incoming property payloads with existing
WI.CSSProperty objects. For pending properties, we only have the property name as a point of verification, which is ripe
for collision when looking at the array of "Pending Properties" that includes commented-out properties.

In addition to properly classifying a "pending" property we need to consider all existing properties for an earlier
comparison between payload properties and WI.CSSProperty objects in WI.DOMNodeStyles.prototype._parseStylePropertyPayload.
We currently are only considering enabled properties for direct matches, even though a commented-out property could
still be a direct match by name and index to an existing property. Failing to make this association results in us
unnecessarily creating a new WI.CSSProperty for the commented out rule, which is turn grows the number of "Pending
Properties" as we toss objects out of the main set of properties for a CSSStyleDeclaration and into the "Pending
Properties", where they then can end up colliding by name inside WI.DOMNodeStyles.prototype._parseStylePropertyPayload.

We instead need to use all of the existing properties to check for a direct name+index match to catch all possible
properties for reuse.

It seems likely that this regressed sometime during the bringup of the Spreadsheet* editor.
https://commits.webkit.org/208214@main, while likely not the cause, renamed the trinity of property arrays as follows:
`properties` -> `enabledProperties`
`allProperties` -> `properties`
`allVisibleProperties` -> `visibleProperties`

It seems ripe for misunderstanding that previously we had both an `allProperties` and a `properties` and I suspect that
the root misunderstanding of this issue stems from that. A quick audit of other uses of `enabledProperties` in our
current code shows generally reasonable usage as far as I can tell.

* LayoutTests/inspector/css/add-css-property-expected.txt:
* LayoutTests/inspector/css/add-css-property.html:

* Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:
* Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:
(WI.DOMNodeStyles.prototype._parseStylePropertyPayload):

Canonical link: https://commits.webkit.org/256410@main