NEW 238232
REGRESSION (r286611): Web Inspector: Style editor doesn't update on Cmd-Z 
https://bugs.webkit.org/show_bug.cgi?id=238232
Summary REGRESSION (r286611): Web Inspector: Style editor doesn't update on Cmd-Z
Nikita Vasilyev
Reported 2022-03-22 15:58:52 PDT
Created attachment 455444 [details] [Video] Bug Steps: 1. Open https://webkit.org 2. Inspect <body> 3. Focus on any CSS property value, such as "font-size: 1.7rem" 4. Modify the value 5. Click elsewhere in Web Inspector to lose focus 6. Press Command-Z to undo changes Expected: Style editor reverts to the original values. Actual: Style editor doesn't update until you re-select the same element.
Attachments
[Video] Bug (4.73 MB, video/quicktime)
2022-03-22 15:58 PDT, Nikita Vasilyev 		no flags
Details
[Video] Heisenbug (78.14 MB, video/quicktime)
2022-05-13 11:59 PDT, Nikita Vasilyev 		no flags
Details
WIP (619 bytes, patch)
2022-05-13 12:12 PDT, Nikita Vasilyev 		nvasilyev: review-
nvasilyev: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-03-22 15:59:04 PDT
<rdar://problem/90664532>
Nikita Vasilyev
Comment 2 2022-05-03 14:16:22 PDT
Bisecting showed that the bug was introduced by Bug 230351 - Web Inspector: Support fuzzy matching in CSS completions. I don't yet understand how.
Nikita Vasilyev
Comment 3 2022-05-03 15:08:38 PDT
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))
Nikita Vasilyev
Comment 4 2022-05-03 15:13:13 PDT
On ToT, SpreadsheetStyleProperty.prototype.update no longer fires on Cmd-Z.
Nikita Vasilyev
Comment 5 2022-05-12 23:27:27 PDT
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.
Nikita Vasilyev
Comment 6 2022-05-13 10:05:41 PDT Comment hidden (obsolete)
Nikita Vasilyev
Comment 7 2022-05-13 11:59:40 PDT
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.
Nikita Vasilyev
Comment 8 2022-05-13 12:01:09 PDT
Which makes it possible that it didn't even regress in r286611, since the whole bisection process was flawed.
Nikita Vasilyev
Comment 9 2022-05-13 12:12:42 PDT
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 😅
Razvan Caliman
Comment 10 2022-05-16 09:05:06 PDT
(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.
Nikita Vasilyev
Comment 11 2022-05-18 10:28:18 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.