Bug 238232

<rdar://problem/90664532>

Bisecting showed that the bug was introduced by Bug 230351 - Web Inspector: Support fuzzy matching in CSS completions. I don't yet understand how.

I reverted the regression on ToT (with `git revert 73faa4680e41784e1aa58c558e8f66eb6652b612` and lots of conflict merges 😅). Once reverted, Cmd-Z property updates the SpreadsheetStyleProperty: update (SpreadsheetStyleProperty.js:165) SpreadsheetStyleProperty (SpreadsheetStyleProperty.js:57) layout (SpreadsheetCSSStyleDeclarationEditor.js:99) _layoutSubtree (View.js:298) _visitViewTreeForLayout (View.js:400) --- requestAnimationFrame --- _scheduleLayoutForView (View.js:361) needsLayout (View.js:177) _propertiesChanged (SpreadsheetCSSStyleDeclarationEditor.js:701) dispatch (Object.js:134) dispatchEventToListeners (Object.js:142) delayed (CSSStyleDeclaration.js:194) --- setTimeout --- update (CSSStyleDeclaration.js:198) _parseStyleDeclarationPayload (DOMNodeStyles.js:692) _parseRulePayload (DOMNodeStyles.js:745) parseRuleMatchArrayPayload (DOMNodeStyles.js:248) fetchedMatchedStyles (DOMNodeStyles.js:266) (anonymous function) (DOMNodeStyles.js:234) _dispatchResponseToCallback (Connection.js:152) _dispatchResponse (Connection.js:122) dispatch (Connection.js:77) dispatchMessageFromTarget (TargetManager.js:176) dispatchMessageFromTarget (TargetObserver.js:47) _dispatchEvent (Connection.js:210) dispatch (Connection.js:79) dispatch (InspectorBackend.js:233) (anonymous function) (MessageDispatcher.js:42) --- setTimeout --- (anonymous function) (MessageDispatcher.js:77) dispatchMessageAsync (InspectorFrontendAPI.js:168) Global Code (Anonymous Script 1 (line 1))

On ToT, SpreadsheetStyleProperty.prototype.update no longer fires on Cmd-Z.

Reverting all `_pendingValue` related logic fixes the issue. https://github.com/WebKit/WebKit/commit/73faa4680e41784e1aa58c558e8f66eb6652b612#diff-945741c84a4fe7494191f06f50221b1efb007c1e2ead88e497b29efc6681feadR84-R85 Looking more into this. So far I'm not convinced that introduction of _pendingValue was a good idea.

Created attachment 459314 [details] [Video] Heisenbug I started to question my sanity but then I realized that the bug sometimes doesn't reproduce if you wait for a few seconds. I have absolutely no idea why.

Which makes it possible that it didn't even regress in r286611, since the whole bisection process was flawed.

Created attachment 459316 [details] WIP I still don't understand what's going on here but this fixes the bug 100% of the time 😅 (In reply to Nikita Vasilyev from comment #8) > Which makes it possible that it didn't even regress in r286611, since the > whole bisection process was flawed. Okay, maybe I shouldn't go this far 😅

(In reply to Nikita Vasilyev from comment #9) > Created attachment 459316 [details] > WIP > > I still don't understand what's going on here but this fixes the bug 100% of > the time 😅 This will introduce another bug. Removing `_applyPendingValue()` here prevents the completion suggestion from being applied on blur when clicking away from the completion menu. The cause for the bug is likely layout-related. The undo operation works reliably all the time as evidenced by changing a value to something outrageous, like `font-size: 90px`, then hitting CMD + Z. But whether the reverted value is correctly reflected in the UI depends on the workflow of navigating away from the text field. Perplexingly, these workflows work: - click away directly to collapse an accordion in the Computed panel, then CMD + Z - hit tab / return until prompted to add a new CSS declaration, then CMD + Z It doesn't help that there are multiple workflows to pick a completion suggestion to update the value (click, enter/tab, right-arrow) and each do something subtly different. I'm continuing to investigate.

(In reply to Razvan Caliman from comment #10) > (In reply to Nikita Vasilyev from comment #9) > > Created attachment 459316 [details] > > WIP > > > > I still don't understand what's going on here but this fixes the bug 100% of > > the time 😅 > > This will introduce another bug. > > Removing `_applyPendingValue()` here prevents the completion suggestion from > being applied on blur when clicking away from the completion menu. This line specifically causes the regression: window.getSelection().setBaseAndExtent(textChildNode, newCaretPosition, textChildNode, newCaretPosition); How, I don't know. I also don't know how to investigate any further, so, perhaps we should just not call that on blur specifically since is isn't needed in that case anyway.