Created attachment 263909 [details] [Animated GIF] Bug See the attached animated GIF. I attempted to type "color:blue". It's a recent regression.
<rdar://problem/23231704>
This is a regression from r191419 (https://bugs.webkit.org/show_bug.cgi?id=147720)
I unrolled r191419 in bug 150537.
Created attachment 288002 [details] Patch
Comment on attachment 288002 [details] Patch LGTM but I want Joe to take a look as well. We really, really need test coverage in this area as it's so complicated.
(In reply to comment #5) > Comment on attachment 288002 [details] > Patch > > LGTM but I want Joe to take a look as well. > > We really, really need test coverage in this area as it's so complicated. Agreed. Does your view testing framework include simulating keyboard events? Should we try using WebDriver (https://webkit.org/blog/6900/webdriver-support-in-safari-10/)?
(In reply to comment #6) > (In reply to comment #5) > > Comment on attachment 288002 [details] > > Patch > > > > LGTM but I want Joe to take a look as well. > > > > We really, really need test coverage in this area as it's so complicated. > > Agreed. Does your view testing framework include simulating keyboard > events? Yes. > Should we try using WebDriver > (https://webkit.org/blog/6900/webdriver-support-in-safari-10/)? No, because that depends on Safari and doesn't fit well with our test runner architecture. The API is very similar though and will eventually use the same UIProcess methods to simulate user actions. More details soon!
<rdar://problem/28175392>
Comment on attachment 288002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288002&action=review > Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:80 > + this._currentCompletion = null; > + this._ignoreChange = false; > + this._ignoreNextCursorActivity = false; > + this._ignoreNextUndo = false; > + this._inCompletionActivity = false; Thanks for declaring these in the constructor. This is quite a list of side states that can easily get mishandled. I really hope we understand when all of these get set/cleared. It just sounds like it would be very easy to end up in a state where oops we have some _ignore set to true that never gets cleared. > Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:518 > if (!force && !backwardScanResult.string && (!baseScanResult || !baseScanResult.string)) { > this.hideCompletions(); > + this._inCompletionActivity = false; > return; > } There is a return below, for extendedCompletionsProvider, that doesn't clear _inCompletionActivity. This boolean seems to have far too many places where it is cleared. This really concerns me. It would be easy to miss one if something is changed in the future. There should be a much clearer entry / exit for this state so we don't need to clear it in ~8 places. Right now it is too complex to follow. r- for now because I can't be certain this doesn't introduce another subtle bug. Is there any reason that _applyCompletionHint can't set the state and clear the state when it is done? Can we create a clearer state machine for CompletionsController?
Comment on attachment 288002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288002&action=review >> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:80 >> + this._inCompletionActivity = false; > > Thanks for declaring these in the constructor. This is quite a list of side states that can easily get mishandled. I really hope we understand when all of these get set/cleared. It just sounds like it would be very easy to end up in a state where oops we have some _ignore set to true that never gets cleared. Yeah that was mainly the motivation for moving these into the constructor. AFAICT, each of them gets cleared, but there may be edgecases :( >> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:518 >> } > > There is a return below, for extendedCompletionsProvider, that doesn't clear _inCompletionActivity. > > This boolean seems to have far too many places where it is cleared. This really concerns me. It would be easy to miss one if something is changed in the future. There should be a much clearer entry / exit for this state so we don't need to clear it in ~8 places. Right now it is too complex to follow. > > r- for now because I can't be certain this doesn't introduce another subtle bug. > > Is there any reason that _applyCompletionHint can't set the state and clear the state when it is done? Can we create a clearer state machine for CompletionsController? So the idea behind _inCompletionActivity is that due to what CodeMirror.changeGeneration(true) does. Basically (assuming I am understanding it right), it tells the history stack to "freeze" itself and start making new history entries from this point forwards. This is necessary because if you rapidly type "color: b" without using the autocomplete, when it next displays the completion popover, it will attempt to undo, meaning that the entire change since the last undo (which is "r: ") will also be removed, leaving "colob" (or something close to that). We can't call CodeMirror.changeGeneration(true) too often or literally each change becomes its own history event. So, _inCompletionActivity is a flag that will track when to call CodeMirror.changeGeneration(true), that being when a new completion is about to be shown and you haven't shown one in a while. Regardless, I think you are right in the issue with extendedCompletionsProvider (which is only JavaScriptRuntimeCompletionProvider AFAICT) ( ͡° ͜ʖ ͡°)
Comment on attachment 288002 [details] Patch Does this patch need any more work before review?
(In reply to comment #11) > Comment on attachment 288002 [details] > Patch > > Does this patch need any more work before review? I don't think so. There may be some work regarding `extendedCompletionsProvider`, but I haven't had a chance to really test it (lots of midterms/projects).
Devin, does this patch still fix something not resolved by rolling out r191419? I couldn't reproduce the bug.
(In reply to comment #13) > Devin, does this patch still fix something not resolved by rolling out > r191419? I couldn't reproduce the bug. I probably should have said this, but this bug was reopened from a past issue (change r191419). Nikita fixed the issue by rolling out r191419, but the major problem remained. The issue was that I changed the way autocompletion suggestions work, going from actually adding the text with a different color to using a CodeMirror marker. A better description is in my comment at <https://webkit.org/b/147975#c3>. The changes I added in the patch on 09/06 was to correctly resolve the problem described there. As far as more work, I don't think there is any work left to do (I will check for sure this weekend and get back), but I did respond to Joe's comments and he may want to comment on them. I am not sure if my solution is the best, but it is all I could think of, so any other reviews are welcome :)
Created attachment 292564 [details] Patch
Comment on attachment 292564 [details] Patch Attachment 292564 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2353708 New failing tests: svg/wicd/test-rightsizing-a.xhtml
Created attachment 292568 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 292998 [details] Patch
Comment on attachment 292998 [details] Patch Clearing flags on attachment: 292998 Committed r208248: <http://trac.webkit.org/changeset/208248>
All reviewed patches have been landed. Closing bug.