Created attachment 358642 [details] [Animated GIF] Bug Steps: 1. Add an event breakpoint. 2. Type "c" 3. Press Arrow Down key 3 times to select "canplay" 4. Type "l" 5. Press Down key Expected: The 1st item from the list should be selected, i.e. "click". Actual: Nothing is selected [Error] Assertion Failed _selectedItemElement (CompletionSuggestionsView.js:222) selectNext (CompletionSuggestionsView.js:92) (anonymous function) (EventBreakpointPopover.js:100)
Created attachment 358657 [details] Patch
I can review this tomorrow, but I want to mention this right now: This patch modifies CompletionSuggestionsView, which is used all over Web Inspector. I've noticed it affected completion in the console. I think I like it :) The bug should be retitled. Completion in the console is far more important than completion in DOM event breakpoints.
Comment on attachment 358657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358657&action=review > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:243 > + if (!isNaN(oldSelectedIndex)) { > + let sway = 0; > + while (true) { > + let itemBefore = oldCompletions[oldSelectedIndex - sway]; > + let itemAfter = oldCompletions[oldSelectedIndex + sway]; > + if (!itemBefore && !itemAfter) { > + selectedIndex = 0; > + break; > + } > + > + let indexBefore = completions.indexOf(itemBefore); > + if (indexBefore !== -1) { > + selectedIndex = indexBefore; > + break; > + } > + > + let indexAfter = completions.indexOf(itemAfter); > + if (indexAfter !== -1) { > + selectedIndex = indexAfter; > + break; > + } > + > + ++sway; > + } > + } I like that you're trying to find previously selected item. It just feels right. r- because I don't understand the use case for `sway > 0`. I don't see how it's useful when typing a character or deleting a character. Please provide the exact steps when it makes sense.
Comment on attachment 358657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358657&action=review >> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:243 >> + } > > I like that you're trying to find previously selected item. It just feels right. > > r- because I don't understand the use case for `sway > 0`. I don't see how it's useful when typing a character or deleting a character. Please provide the exact steps when it makes sense. This code is trying to find the closest item to the selected item that existed in the previous list _and_ exists in the new list. 1. create an event breakpoint 2. type "c" to filter by event names starting with "c" 3. arrow down to "change" 4. type "l" to filter by event names starting with "cl" Since "change" doesn't start with "cl", we should shift the selection to the closest item that starts with "cl" to preserve the _relative_ index of selection. To continue the example above, "change" was towards the beginning(ish) of the list of "c", which means that we should try to select something towards the beginning(ish) of the list of "cl". Selecting the identical index ("change" is 5) doesn't make a lot of sense because there are only two items for "cl". Selecting the last item also doesn't make a huge amount of sense because that would feel like "clamping", especially since each character is guaranteed to make the list shorter (you'd always end up selecting the last item after each type, which is just weird). The approach I decided on was to try to "shift" the selection to something "familiar" that was (and is) nearby so that it instead feels like you've just "drilled down" into the list and moved the selection to match. One thing I've noticed while typing this is that CSS (and JavaScript IIRC) completions are sorted alphabetically, so there's really only need to move in one direction (e.g. "cl" is strictly after "change", so there's no reason to look before), but considering that `WI.CompletionSuggestionView` is a more agnostic system (e.g. it doesn't care about whether the list is sorted) it might be best to check both ways.
Comment on attachment 358657 [details] Patch Reseting back to "r?". (In reply to Devin Rousso from comment #4) > This code is trying to find the closest item to the selected item that > existed in the previous list _and_ exists in the new list. > 1. create an event breakpoint > 2. type "c" to filter by event names starting with "c" > 3. arrow down to "change" > 4. type "l" to filter by event names starting with "cl" Only two events start with "cl": - click - close I agree that we should select "click". However, I think we should just select the 1st item on the list. If you replace the 3rd step with "arrow down to 'cut', which is the last item on the list", do you think we it's beneficial to select "close" instead of "click"?
(In reply to Nikita Vasilyev from comment #5) > If you replace the 3rd step with "arrow down to 'cut', which is the last item on the list", do you think we it's beneficial to select "close" instead of "click"? If we instead had selected "complete" (which is right after "close"), it would seem odd to me to shift that far "away" from what was previously selected. Furthermore, if we then ⌫ (e.g. accidentally typed "l"), "click" is further away from "complete" and would require more work to get back to where we started. This would be even more of an issue if there were more events that started with "cl" (e.g. all the "accessible*" events vs "ad*").
Comment on attachment 358657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358657&action=review Looks good overall. Some comments below. > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:80 > + setTimeout(() => { Have you considered using requestAnimationFrame? > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:221 > + while (true) { Nit: limit sway to some reasonable number. Completion in the console and the styles sidebar only shows 10 items on screen at a time, so I'd pick 10. > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:223 > + let itemBefore = oldCompletions[oldSelectedIndex - sway]; > + let itemAfter = oldCompletions[oldSelectedIndex + sway]; Consider the case when the oldSelectedIndex is at the very end of the list. itemAfter would always be undefined and ... > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:235 > + let indexAfter = completions.indexOf(itemAfter); ...we would run completions.indexOf(undefined). We should do something like: itemAfter ? completions.indexOf(itemAfter) : -1 Ditto for itemBefore.
(In reply to Devin Rousso from comment #4) > One thing I've noticed while typing this is that CSS (and JavaScript IIRC) > completions are sorted alphabetically, so there's really only need to move > in one direction (e.g. "cl" is strictly after "change", so there's no reason > to look before), but considering that `WI.CompletionSuggestionView` is a > more agnostic system (e.g. it doesn't care about whether the list is sorted) > it might be best to check both ways. I'd prefer to check only one direction and change the code when (if ever) we have unsorted completions.
<rdar://problem/48916980>
Comment on attachment 358657 [details] Patch Marking as r- to remove from the queue, given as there's no consensus.