Web Inspector: Styles: pressing Down key on empty value field shouldn't discard completion popover

[Nikita Vasilyev]

Created attachment 358236 [details]
[Animated GIF] Bug

Steps:
1. Add "font-family" property.
2. Press Tab to focus on the property value field.
3. Press Down key.

Expected:
The 1st item is selected, the completion popover stays visible.

Actual:
The 1st item is selected, the completion popover discarded.

[Radar WebKit Bug Importer]

<rdar://problem/47016036>

[Nikita Vasilyev]

Created attachment 358237 [details]
Patch

[Nikita Vasilyev]

Created attachment 358238 [details]
[Animated GIF] With patch applied

[Devin Rousso]

Comment on attachment 358237 [details]
Patch

r-, as this doesn't work if the completion causes a wrapping to the next line:

1. inspect <https://webkit.org>
2. show the Elements tab
3. make the Styles sidebar as small (width) as possible
4. add a new property with "-apple-pay-button-style" as the name
5. tab to the value editor
6. press down
 => the "black" value causes the property to wrap, which hides the suggestions list

Rather than try to calculate the position of the caret, perhaps we can use a capturing "scroll" listener on the document/window that checks to see if the target is an ancestor of the suggestions view, and if so removes the listener and hides the suggestions view.  This would prevent other hide triggers (e.g. resizing the Styles sidebar while the suggestions view is showing causes it to hide if the property no longer wraps) and simplify the logic (albeit at the "expense" of a capturing event listener).

[Nikita Vasilyev]

Good catch! I'll try listening for scrolling.

[Nikita Vasilyev]

Resizing sidebar doesn't cause any scroll events but it moves the anchor element of the popover.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #6)
> Resizing sidebar doesn't cause any scroll events but it moves the anchor element of the popover.
Perhaps instead of relying on the cursor position, use another anchor?  The property's "row" element is unlikely to move it's origin (top-left) position when editing the name/value.  It would just grow down.

[Nikita Vasilyev]

Created attachment 358986 [details]
Patch

[Nikita Vasilyev]

Created attachment 358987 [details]
[Video] With patch applied 2

[Devin Rousso]

Comment on attachment 358986 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:183
> +            let clientRect = element.getBoundingClientRect();

This is pretty expensive, especially doing it every 200ms (I know we had a `setInterval` earlier, but I don't really like that as well).  Is there any way we can provide a scrollable element to some sort of `showUntilElementScrolls` function, which would take a scroll container and add/remove a "scroll" event listener to do this logic?

    showUntilElementScrolls(scrollElement, targetElement)
    {
        scrollElement.addEventListener("scroll", (event) => {
            this.hide();
        }, {once: true});

        this.show(targetElement);
    }

You'd then have to get the scrollable element, which I believe is the "Rules" sidebar panel, so that may require some delegate calls, but I think this is a much more performant and less "inquisitive" approach to checking whether we're in the same spot.  Perhaps we can even add a MutationObserver to see if the target element gets removed, such as during a refresh, but I also think that if the caller of `showUntilElementScrolls` decides to remove the target out from underneath `WI.CompletionSuggestionsView`, that's their fault and they should be responsible enough to hide the popover too.

[Nikita Vasilyev]

Again, resizing styles sidebar doesn't cause any scroll events.

Yes, it's potentially expensive but that's what we do already — my patch doesn't make it slower. Can we optimize performance separately from this bug? I've been playing around with intersection observers, I might be able to use utilize them here.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #11)
> Again, resizing styles sidebar doesn't cause any scroll events.
`WI.SpreadsheetRulesStyleDetailsPanel` can leverage `sizeDidChange` to cover this support.

> Yes, it's potentially expensive but that's what we do already — my patch doesn't make it slower. Can we optimize performance separately from this bug? I've been playing around with intersection observers, I might be able to use utilize them here.
I guess we already are calling `getBoundingClientRect` right now inside `WI.SpreadsheetTextField.prototype._getCaretRect`, but frankly I'd rather improve than "keep the status quo".  Too often it is said "I'll do that in a followup" and then it never happens (I'm guilty of this), so I'd ask that you take a shot at making it better before delaying to a followup.

[Nikita Vasilyev]

I just measured how long does getBoundingClientRect call take.

    console.time("getBoundingClientRect");
    let clientRect = element.getBoundingClientRect();
    console.timeEnd("getBoundingClientRect");

It was between 0.02ms and 0.07ms when adding a new property to <body> on webkit.org.
This is pretty fast and I don't think this needs to be optimized.

[Devin Rousso]

Comment on attachment 358986 [details]
Patch

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

r=me, I'm still not a fan of keeping `setInterval` and `getBoundingClientRect`, but in the interest of time, as well as the fact that this shouldn't be any worse than what we had before, I'll say fine.  Please open a followup to see if we can address these issues (e.g. "Web Inspector: WI.CompletionSuggestionsView shouldn't use setInterval/getBoundingClientRect to determine if the target element has moved").

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:36
> +        this._moveIntervalIdentifier = 0;

NIT: we normally set identifiers to `undefined` (or `null`).

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:260
> +        this._moveIntervalIdentifier = 0;

Ditto (>36).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:308
> +                this._suggestionsView.scrollTop = scrollTop;

Can this be moved inside `WI.CompletionSuggestionView.prototype.show`?  It seems odd to require the caller to re-scroll the element when the element is added within `WI.CompletionSuggestionView`.

[Nikita Vasilyev]

Created attachment 359065 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 359065 [details]
Patch

Clearing flags on attachment: 359065

Committed r239935: <https://trac.webkit.org/changeset/239935>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.