Bug 193098 - Web Inspector: Styles: pressing Down key on empty value field shouldn't discard completion popover
Summary: Web Inspector: Styles: pressing Down key on empty value field shouldn't disca...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-02 18:21 PST by Nikita Vasilyev
Modified: 2019-01-14 13:08 PST (History)
4 users (show)

See Also:


Attachments
[Animated GIF] Bug (161.43 KB, image/gif)
2019-01-02 18:21 PST, Nikita Vasilyev
no flags Details
Patch (4.45 KB, patch)
2019-01-02 18:30 PST, Nikita Vasilyev
drousso: review-
Details | Formatted Diff | Diff
[Animated GIF] With patch applied (108.38 KB, image/gif)
2019-01-02 18:31 PST, Nikita Vasilyev
no flags Details
Patch (8.87 KB, patch)
2019-01-12 00:17 PST, Nikita Vasilyev
drousso: review+
Details | Formatted Diff | Diff
[Video] With patch applied 2 (2.68 MB, video/quicktime)
2019-01-12 00:18 PST, Nikita Vasilyev
no flags Details
Patch (8.74 KB, patch)
2019-01-14 12:29 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-01-02 18:21:31 PST
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.
Comment 1 Radar WebKit Bug Importer 2019-01-02 18:21:53 PST
<rdar://problem/47016036>
Comment 2 Nikita Vasilyev 2019-01-02 18:30:10 PST
Created attachment 358237 [details]
Patch
Comment 3 Nikita Vasilyev 2019-01-02 18:31:40 PST
Created attachment 358238 [details]
[Animated GIF] With patch applied
Comment 4 Devin Rousso 2019-01-02 21:23:01 PST
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).
Comment 5 Nikita Vasilyev 2019-01-02 22:55:29 PST
Good catch! I'll try listening for scrolling.
Comment 6 Nikita Vasilyev 2019-01-03 16:27:25 PST
Resizing sidebar doesn't cause any scroll events but it moves the anchor element of the popover.
Comment 7 Devin Rousso 2019-01-10 21:30:32 PST
(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.
Comment 8 Nikita Vasilyev 2019-01-12 00:17:56 PST
Created attachment 358986 [details]
Patch
Comment 9 Nikita Vasilyev 2019-01-12 00:18:44 PST
Created attachment 358987 [details]
[Video] With patch applied 2
Comment 10 Devin Rousso 2019-01-12 19:37:37 PST
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.
Comment 11 Nikita Vasilyev 2019-01-12 19:53:07 PST
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.
Comment 12 Devin Rousso 2019-01-12 20:03:39 PST
(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.
Comment 13 Nikita Vasilyev 2019-01-12 20:28:18 PST
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.
Comment 14 Devin Rousso 2019-01-14 12:06:34 PST
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`.
Comment 15 Nikita Vasilyev 2019-01-14 12:29:54 PST
Created attachment 359065 [details]
Patch
Comment 16 WebKit Commit Bot 2019-01-14 13:08:03 PST
Comment on attachment 359065 [details]
Patch

Clearing flags on attachment: 359065

Committed r239935: <https://trac.webkit.org/changeset/239935>
Comment 17 WebKit Commit Bot 2019-01-14 13:08:05 PST
All reviewed patches have been landed.  Closing bug.