Bug 182027 – REGRESSION (r226994): Web Inspector: Styles: Suggestions popover floats in top-left corner of Web Inspector after tabbing

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

* STEPS TO REPRODUCE
1. Select the Elements tab
2. Show the details sidebar (⌥⌘0)
3. Select Styles
4. Scroll down to “html, body {“
5. Click to the right of the left brace
6. Press Tab

* EXPECTED RESULTS
- Keyboard focus should change to next attribute

* ACTUAL RESULTS
- Suggestions popover opens in the top-left corner of Web Inspector

<rdar://problem/36794496>

[Nikita Vasilyev]

Broke in https://trac.webkit.org/changeset/226994/webkit :(

What's happening?

SpreadsheetTextField.js:

    _getCaretRect(prefix, completionPrefix)
        ...
        let clientRect = this._element.getBoundingClientRect();

this._element is detached from DOM.
clientRect is {x: 0, y: 0, width: 0, height: 0}

[Nikita Vasilyev]

Things I learned:

    this._element.addEventListener("focus", () => console.info("focused") }
    this._element.focus();
    console.info("select text");

I expected this to be in the following order:

    -> select text
    -> focused

But in fact it's this:

    -> focused
    -> select text

WI.SpreadsheetTextField.prototype.startEditing:

    this._element.focus();
    this._selectText();

    this._updateCompletions();

Changing focus triggers blur and focus events. this._element may get detached from DOM by the time this._updateCompletions is called.

[Nikita Vasilyev]

Created attachment 332182 [details]
Patch

[Matt Baker]

(In reply to Nikita Vasilyev from comment #3)
> Created attachment 332182 [details]
> Patch

This fixes the popover positioning issue, but I'm not sure why we're trying to show the completion list in the first place. Showing autocomplete every time you tab into a property name seems overkill. I noticed Chrome does not have this behavior.

I expected the behavior to be:
1. Click to the right of "html, body {"
=> Caret moves to new blank property
2. Press tab
=> Blank property is removed, selection moves to next property name

[Nikita Vasilyev]

(In reply to Matt Baker from comment #4)
> (In reply to Nikita Vasilyev from comment #3)
> > Created attachment 332182 [details]
> > Patch
> 
> This fixes the popover positioning issue, but I'm not sure why we're trying
> to show the completion list in the first place. Showing autocomplete every
> time you tab into a property name seems overkill. I noticed Chrome does not
> have this behavior.
> 
> I expected the behavior to be:
> 1. Click to the right of "html, body {"
> => Caret moves to new blank property
> 2. Press tab
> => Blank property is removed, selection moves to next property name

It shows autocomplete when focusing on "margin" because it's a prefix for "margin-bottom", "margin-left", and etc. When focusing on, say, "vertical-align" it doesn't show the suggestion popover because "vertical-align" isn't a prefix for any properties.

I don't have a strong opinion on whether we should only display the suggestion popover after typing. During one of the design meetings people like the current version more.

[Matt Baker]

(In reply to Nikita Vasilyev from comment #5)
> I don't have a strong opinion on whether we should only display the
> suggestion popover after typing. During one of the design meetings people
> like the current version more.

Then the people have spoken!

[Matt Baker]

Comment on attachment 332182 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:381
> +            if (!caretRect) {

Instead of having _getCaretRect return `null` in this special case, could we just early return if this._element isn't in the DOM? The fix would better match your Changelog comment: "Don't show autocomplete popover for an element that is detached from DOM."

[Matt Baker]

Comment on attachment 332182 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:10
> +        popover, resulting the popover to be displayed at the top left corner.

Nit: clean up the wording here: "resulting in the popover being displayed at..."

[Nikita Vasilyev]

Created attachment 332205 [details]
Patch

[Nikita Vasilyev]

Created attachment 332206 [details]
Patch

[Matt Baker]

Comment on attachment 332206 [details]
Patch

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

r=me, with one more style comment

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:368
> +        console.assert(this._element.isConnected, "_updateCompletions got called after SpreadsheetTextField was removed from the DOM");

Nit: assertion message should include a period. It can also be shortened to "SpreadsheetTextField already removed from the DOM.". The context will be evident from the console log, and in general we should avoid including function/variable names in log messages. If this code is relocated or the function name changes, someone would have to remember to update the message.

[Nikita Vasilyev]

Created attachment 332209 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 332209 [details]
Patch

Clearing flags on attachment: 332209

Committed r227585: <https://trac.webkit.org/changeset/227585>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.