Web Inspector: Styles: Autocomplete should support mid-line completions

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

Completions in the Styles panel can currently only be performed at the end of a value, with no support for the cursor occurring at any other location. We should change this to behave in a more expected manner by performing completions at the caret's position. Bug 227097 began this work, but is blocked by a number of other issues regarding the reliability of completions being committed.

[Radar WebKit Bug Importer]

<rdar://problem/79801095>

[Patrick Angle]

Created attachment 432644 [details]
WIP v0.1 - Rebased version of Bug 227097's Patch v1.3

[Patrick Angle]

The biggest issue with this patch currently is that it has made an existing issue more apparent where properties in the Style panel no longer match the actual style or, worse, style text becomes completely corrupted.

Steps to reproduce:
Go to webkit.org and inspect the `A fast, open source web browser engine.` text.
1. Add a `color:` property to the `Style Attribute` section.
2. Set the color value to `aqua`.
3. Click out and back into the the value field (it will highlight the entire value).
4. Press the `backspace`, `a`, `q`, `enter` keys in that order.

Results: Not every time, but often, observe the value being displayed in the sidebar isn’t the value that displays as inline on the node in the DOM tree. Selecting a different element seems to kick everything to the correct value of aqua.

If the property is added between two other properties, these steps will occasionally reproduce the style text corruption bug as well.

[Nikita Vasilyev]

(In reply to Patrick Angle from comment #3)
> The biggest issue with this patch currently is that it has made an existing
> issue more apparent where properties in the Style panel no longer match the
> actual style or, worse, style text becomes completely corrupted.
> 
> Steps to reproduce:
> Go to webkit.org and inspect the `A fast, open source web browser engine.`
> text.
> 1. Add a `color:` property to the `Style Attribute` section.
> 2. Set the color value to `aqua`.
> 3. Click out and back into the the value field (it will highlight the entire
> value).
> 4. Press the `backspace`, `a`, `q`, `enter` keys in that order.
> 
> Results: Not every time, but often, observe the value being displayed in the
> sidebar isn’t the value that displays as inline on the node in the DOM tree.
> Selecting a different element seems to kick everything to the correct value
> of aqua.
> 
> If the property is added between two other properties, these steps will
> occasionally reproduce the style text corruption bug as well.

As you and I just checked, this is resolved by Bug 178835 - Web Inspector: Styles: format style declarations after editing. The issue was with incorrect ranges (negative numbers when the value is empty resulted in corrupt state) and my patch in Bug 178835 doesn't rely on ranges any more.

[Razvan Caliman]

Created attachment 440784 [details]
Patch v1.0

Bugs 178835 and 227097 landed. Requesting review on behalf of [e-mail]

[Devin Rousso]

Comment on attachment 440784 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=440784&action=review

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:70
> +        return Array.from(this._element.childNodes)

NIT: While I do appreciate the nice-ness of this code, it is iterating over the children _four_ times.  I doubt that's an issue since we're probably not likely to have more than a dozen or so children, but it's still something to keep in mind.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:202
> +            if (this.suggestionHint.length)

NIT: Do you actually need to check this?  I feel like we can always call `discardCompletion`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:234
> +            else

Style: no need for `else` if the corresponding `if` has a `return`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:298
> +                this._preventDiscardingCompletionsOnKeyUp = true;

Does this need to be set back to `false` somewhere?  Usually for things like this we either "consume" the flag in the corresponding event handler (i.e. `_handleKeyUp`) or always reset it at the top of this event handler.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:385
> +        if (this.suggestionHint.length || this._suggestionsView.visible) {

ditto (:202)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:387
> +            this._delegate?.spreadsheetTextFieldDidChange?.(this);

this should already be called by `discardCompletion`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:396
> +        this._preventDiscardingCompletionsOnKeyUp = true;

ditto (:298)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:521
> +    _applyCompletionHint({moveCaretToEndOfCompletion} = {})

In what situation would we _not_ want to `moveCaretToEndOfCompletion` when applying a completion?  I'd think that applying a completion would _always_ move the caret to the end of that completion so that the user can start typing something new instead of editing the middle of the value they just completed.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:526
> +        let newCaretPosition = moveCaretToEndOfCompletion ? this._getCaretPosition() + this.suggestionHint.length : null;

NIT: I'd inline this

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:550
> +        if (!this.suggestionHint.length)
> +            return;

NIT: All callers either check or set this value, so adding an early-return is kinda unnecessary IMO (and the `console.assert` should be enough to ensure future correctness).  Alternatively, you could rename this to `_attachSuggestionHintIfPossible` and remove all the checks at the callsites.

[Patrick Angle]

Created attachment 440837 [details]
Patch v1.1

[Patrick Angle]

Comment on attachment 440784 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=440784&action=review

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:70
>> +        return Array.from(this._element.childNodes)
> 
> NIT: While I do appreciate the nice-ness of this code, it is iterating over the children _four_ times.  I doubt that's an issue since we're probably not likely to have more than a dozen or so children, but it's still something to keep in mind.

Fair point - I've refactored to not iterate over the array 4x.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:521
>> +    _applyCompletionHint({moveCaretToEndOfCompletion} = {})
> 
> In what situation would we _not_ want to `moveCaretToEndOfCompletion` when applying a completion?  I'd think that applying a completion would _always_ move the caret to the end of that completion so that the user can start typing something new instead of editing the middle of the value they just completed.

Handling the `blur` event (`_handleBlur`) we don't want to set the cursor's position, since that would bring focus right back to the text field as soon as it was `blur`red.

[Patrick Angle]

Created attachment 443703 [details]
Patch v1.1.1 - Remove stray commented-out code

[Devin Rousso]

Comment on attachment 443703 [details]
Patch v1.1.1 - Remove stray commented-out code

View in context: https://bugs.webkit.org/attachment.cgi?id=443703&action=review

r=me, neat! :)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:393
> +        if (!this._suggestionsView || this._preventDiscardingCompletionsOnKeyUp || this._keyDownCaretPosition === this._getCaretPosition() || this._keyDownCaretPosition === -1)

I feel like only the things related to `_keyDownCaretPosition` are actually related to this comment.  Perhaps we can make a separate `if (this._preventDiscardingCompletionsOnKeyUp)` and `if (!this._suggestionsView)` so that things are a bit nicer to read?

NIT: I'd put `this._keyDownCaretPosition === -1` earlier so that we only do `this._getCaretPosition()` as a last resort

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:475
> +        // The window's selection range will only contain the current line's positioning in multiline text, so a new

NIT: Can we move this comment down a bit so it's closer to the relevant code?  The early-return doesn't appear to be related to this.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:548
> +            console.assert(textChildNode instanceof Text, textChildNode);

I don't think it's actually possible for this to not be true given that you just set the `textContent` 🤔

[Patrick Angle]

Created attachment 444077 [details]
Patch

[Patrick Angle]

Comment on attachment 444077 [details]
Patch

Doing final manual testing on this particularly hot part of the Web Inspector UI.

[Patrick Angle]

Created attachment 444267 [details]
Patch v1.2.1 - For landing

[EWS]

Committed r285851 (244277@main): <https://commits.webkit.org/244277@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444267 [details].