[webkit-changes] [WebKit/WebKit] 84150e: Web Inspector: New CSS property unexpectedly dropp...

[Razvan Caliman - Apple]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 84150ea85574551811c8802fe27fa252bc571d31
      https://github.com/WebKit/WebKit/commit/84150ea85574551811c8802fe27fa252bc571d31
  Author: Razvan Caliman <rcaliman at apple.com>
  Date:   2023-03-20 (Mon, 20 Mar 2023)

  Changed paths:
    M Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js
    M Source/WebInspectorUI/UserInterface/Views/SpreadsheetRuleHeaderField.js

  Log Message:
  -----------
  Web Inspector: New CSS property unexpectedly dropped from empty CSS rule when tabbing through or editing selector
https://bugs.webkit.org/show_bug.cgi?id=245768

Reviewed by Patrick Angle.

There are two issues here:
1) changing the selector of an empty CSS rule causes the first added CSS property to be dropped.
2) tabbing through the editable text field for a CSS selector marks it as changed even when it's not.

The core problem is the order of operations.

Tabbing out of a CSS selector text field first calls
`SpreadsheetCSSStyleDeclarationSection.spreadsheetRuleHeaderFieldWillNavigate()`
from the `SpreadsheetRuleHeaderField._handleKeyDown` handler.

For an empty CSS rule, this creates a new blank CSS property with:
`SpreadsheetCSSStyleDeclarationEditor.startEditingFirstProperty()` -> `CSSStyleDeclaration.newBlankProperty()`.

At this point, the frontend model for the CSS rule contains a `CSSStyleDeclaration` with one `CSSProperty`.

In quick succession, the second operation happens when the selector text field loses focus:
`SpreadsheetCSSStyleDeclarationSection.spreadsheetRuleHeaderFieldDidCommit()`
from the `SpreadsheetRuleHeaderField._handleBlur` handler.

If the selector text has changed, a request is sent to the backend via `DOMNodeStyles.changeRuleSelector()`.
A request is then made for the latest matching styles via `DOMNodeStyles.refresh()`.

For an empty CSS rule, there are no matching styles. As a result, the `CSSStyleDeclaration` of the corresponding
CSS rule is updated with an empty list of CSS properties in `DOMNodeStyles._parseStyleDeclarationPayload()`:

```
if (style) {
    style.update(text, properties, styleSheetTextRange);
    return style;
}
```

This has the effect of orphaning the new blank `CSSProperty` introduced earlier.

`CSSProperty._updateOwnerStyleText()` is called to persist the `CSSProperty` to the backend.
This calls `CSSStyleDeclaration.generateFormattedText()` to serialize the whole CSS declaration block.
But our `CSSStyleDeclaration` was just emptied of properties by the update in
`DOMNodeStyles._parseStyleDeclarationPayload()` so it falls back to returning an empty string:

```
let styleText = "";
...
let properties = this._styleSheetTextRange ? this.visibleProperties : this._properties;
...
return styleText;
```

When the `CSSProperty` commits with an empty name and value, the matching styles obtained
in response to `DOMNodeStyles.refresh()` are empty, and the orphaned property is dropped.

Trying again with the newly created blank CSS property will work because this time
there's no race with `DOMNodeStyles.refresh()` from the selector change.

To fix the first issue, we must ensure that the operation to update the selector
and the style refresh it causes occurs before adding a new CSS property to avoid
the style refresh invalidating the models on the frontend.

To fix the second issue, we don't call `SpreadsheetRuleHeaderField.stopEditing()` in the
keydown handler for Tab or Enter  because that resets `_valueBeforeEditing` which causes
the check in  `SpreadsheetRuleHeaderField._handleBlur` to always consider the selector
as having been changed, thus triggering the operations described above.

* Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:
(WI.SpreadsheetCSSStyleDeclarationSection):
(WI.SpreadsheetCSSStyleDeclarationSection.prototype._handleSpreadsheetSelectorFieldDidCommit):
(WI.SpreadsheetCSSStyleDeclarationSection.prototype._handleSpreadsheetSelectorFieldWillNavigate):
* Source/WebInspectorUI/UserInterface/Views/SpreadsheetRuleHeaderField.js:
(WI.SpreadsheetRuleHeaderField):
(WI.SpreadsheetRuleHeaderField.prototype.stopEditing):
(WI.SpreadsheetRuleHeaderField.prototype._handleBlur):
(WI.SpreadsheetRuleHeaderField.prototype._handleKeyDown):

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