* 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>
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}
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.
Created attachment 332182 [details] Patch
(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
(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.
(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!
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."
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..."
Created attachment 332205 [details] Patch
Created attachment 332206 [details] Patch
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.
Created attachment 332209 [details] Patch
Comment on attachment 332209 [details] Patch Clearing flags on attachment: 332209 Committed r227585: <https://trac.webkit.org/changeset/227585>
All reviewed patches have been landed. Closing bug.