Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: 0d990da3c01519907affff83cfdee71283f2ef4f
https://github.com/WebKit/WebKit/commit/0d990da3c01519907affff83cfdee71283f2ef4f
Author: Patrick Angle <[email protected]>
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
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
