Summary: | Web Inspector: Styles Redesign: data corruption when updating values quickly | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2017-11-08 18:14:22 PST
This wasn't a problem in the old styles sidebar because it treated text in the view (CodeMirror instance) as the source of truth. One way of fixing this would be to ignore WI.CSSStyleDeclaration updates from the backend when its corresponding view is being edited (e.g. one of CSS properties are in focus). WI.CSSStyleDeclaration gets updates in the following scenarios: 1. CSS edited in Styles panel. 2. CSS edited in Resources tab. 3. CSS modified with JS. This strategy should work well with (1) and (2), but not with (3). Since we currently don't receive updates from the backend when CSSOM changes are made anyway, I think it's an okay strategy. Bug 92644 - Web Inspector: CSS rule changes made via CSSStyleSheet DOM API are not reflected in the Styles pane Bug 141450 - Web Inspector: Better support for CSSOM StyleSheet mutations (insertRule/deleteRule) Locking (ignoring WI.CSSStyleDeclaration updates from the backend while it's being edited) has issues: the front-end doesn't know if a newly added CSS property is valid or overridden. Back to the drawing board :( Created attachment 328921 [details]
Patch
Created attachment 328922 [details]
Patch (debug)
Created attachment 328923 [details]
[Animated GIF] With debug patch applied
The debug patch highlights SpreadsheetCSSStyleDeclarationEditor with pink background when CSSStyleDeclaration is locked.
Otherwise, it's exactly like the patch for review.
Created attachment 328924 [details]
[Animated GIF] With debug patch applied
The debug patch highlights SpreadsheetCSSStyleDeclarationEditor with pink background when CSSStyleDeclaration is locked.
Otherwise, it's exactly like the patch for review.
Created attachment 328926 [details]
Patch for review
Created attachment 329870 [details]
Patch for review
Rebaselined.
Comment on attachment 329870 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=329870&action=review r-, only because this definitely needs some tests. The logic otherwise sounds fine, but having tests (specifically on CSSStyleDeclaration) would really help clarify what will happen in the various cases: - not locked, style changes via page's JS - locked, style changes via page's JS - locked, style changes via WebInspector - locked, style changes via WebInspector and page's JS (both orderings) - etc. It should be pretty simple to verify just by checking the style's text. > Source/WebInspectorUI/ChangeLog:20 > + To correctly display invalid and overridden properties, the backend allowed to update I think you're missing a word in this sentence, like "is". > Source/WebInspectorUI/ChangeLog:50 > + When selector is focused, clicking on the white-space should not add a new blank property. I'd move this comment after the final item for this file, so it's a bit easier to read. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:105 > + update(text, properties, styleSheetTextRange, dontFireEvents, suppressLock) It'd be nice of we could move some of these "optional" properties to an object parameter, update(text, properties, styleSheetTextRange, options = {}) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:247 > + // spreadsheetStyleProperty delegate NIT: SpreadsheetStyleProperty Also, these should probably go underneath the "Public" section. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:354 > + if (this._delegate && typeof this._delegate.stylePropertyInlineSwatchActivated === "function") { I don't think we ever have a situation where `this._delegate` is invalid. I think you can remove this. Comment on attachment 329870 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=329870&action=review >> Source/WebInspectorUI/ChangeLog:20 >> + To correctly display invalid and overridden properties, the backend allowed to update > > I think you're missing a word in this sentence, like "is". Grammar: "the backend allowed to" => "the backend is allowed to" > Source/WebInspectorUI/ChangeLog:23 > + front-end changes. I think you also said this update when text is the same is also useful to update other states like validity of the property. Might be worth including something along that line. --- In principle this approach sounds fine. If you have a situation like: Front-end: (1)-(2)---(3)-(blur) Back-end: (1)-------------(2)-(3) We would end up applying update (2) and (3) right? Hopefully without any visual flicker. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:74 > + if (this._ownerStyle && this._ownerStyle.locked && text !== this._text) > + return; A comment here, like in the ChangeLog, would make sense about why we update even though we are locked. It is non-obvious. Comment on attachment 329870 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=329870&action=review >> Source/WebInspectorUI/ChangeLog:23 >> + front-end changes. > > I think you also said this update when text is the same is also useful to update other states like validity of the property. Might be worth including something along that line. > > --- > > In principle this approach sounds fine. > > If you have a situation like: > > Front-end: (1)-(2)---(3)-(blur) > Back-end: (1)-------------(2)-(3) > > We would end up applying update (2) and (3) right? Hopefully without any visual flicker. Good catch! Yes, it would apply (2) and (3) when changing a value and clicking outside of the styles sidebar. Ideally, it should be just (3). On my machine, most updates from the backend happen <25ms after the change on the front-end, so it's hard to be change the value and click outside of the styles sidebar that quickly. So far I haven't done any performance testing on extremely large documents and slow hardware, but it may worth doing in the future. When changing a value and tabbing to the next property name, a blur event happens first, shortly followed by a focus event. The delay between the two is 0-1ms on my machine. I don't know how blur and focus events are implemented in WebKit. It may be possibly to receive a backend payload after the blur event but before the focus event, but I haven't been able to reproduce it so far. Created attachment 331260 [details]
Patch for review
(In reply to Devin Rousso from comment #11) > Comment on attachment 329870 [details] > Patch for review > > View in context: > https://bugs.webkit.org/attachment.cgi?id=329870&action=review > > r-, only because this definitely needs some tests. The logic otherwise > sounds fine, but having tests (specifically on CSSStyleDeclaration) would > really help clarify what will happen in the various cases: > > - not locked, style changes via page's JS > - locked, style changes via page's JS > - locked, style changes via WebInspector > - locked, style changes via WebInspector and page's JS (both orderings) > - etc. Some of these are tricky to test. For example: "locked, style changes via page's JS". Style change happens asynchronously and it doesn't trigger any related events (such as WI.CSSStyleDeclaration.Event.PropertiesChanged). I could have listened that WI.CSSStyleDeclaration.Event.PropertiesChanged does NOT get called in the next 500ms or so but that could be flaky and/or slow, and I decided not to do it. Using this patch I still saw occasional assertions. Harmless I assume since everything seemed to behave as expected: [Error] Assertion Failed update (CSSStyleDeclaration.js:175) _parseStyleDeclarationPayload (DOMNodeStyles.js:676) _parseRulePayload (DOMNodeStyles.js:758) parseRuleMatchArrayPayload (DOMNodeStyles.js:103) fetchedMatchedStyles (DOMNodeStyles.js:126) (anonymous function) (DOMNodeStyles.js:88) _dispatchResponseToCallback (Connection.js:146) _dispatchResponse (Connection.js:116) dispatch (Connection.js:69) dispatch (InspectorBackend.js:152) dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:42) (In reply to Joseph Pecoraro from comment #16) > Using this patch I still saw occasional assertions. Harmless I assume since > everything seemed to behave as expected: > > [Error] Assertion Failed > update (CSSStyleDeclaration.js:175) > _parseStyleDeclarationPayload (DOMNodeStyles.js:676) > _parseRulePayload (DOMNodeStyles.js:758) > parseRuleMatchArrayPayload (DOMNodeStyles.js:103) > fetchedMatchedStyles (DOMNodeStyles.js:126) > (anonymous function) (DOMNodeStyles.js:88) > _dispatchResponseToCallback (Connection.js:146) > _dispatchResponse (Connection.js:116) > dispatch (Connection.js:69) > dispatch (InspectorBackend.js:152) > dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:42) This is an old assertion: // Don't fire the event if there is text and it hasn't changed. if (oldText && this._text && oldText === this._text) { // We shouldn't have any added or removed properties in this case. console.assert(!addedProperties.length && !removedProperties.length); Not sure why it's triggering, investigating. Comment on attachment 331260 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=331260&action=review This seems reasonable, lets live on it. r=me > LayoutTests/inspector/css/modify-css-property.html:19 > + name: "Update value when CSSStyleDeclaration is unlocked", I'd suggest "not-locked". "unlocked" sounds like the verb, so it comes across as "when it is unlocked", which is not actually what is happening here. > LayoutTests/inspector/css/modify-css-property.html:126 > + InspectorTest.evaluateInPage("makeWide()"); Style: We've been using template strings for code, especially in cases like this: evaluateInPage(`code()`) > LayoutTests/inspector/css/modify-css-property.html:128 > + resolve(); This test can resolve in multiple ways. Here (immediately) or above with awaitEvent. I think you should keep only the awaitEvent version, since that happens after this one and includes expectations. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:52 > + this.element.addEventListener("focus", () => this.focused = true, true); Style: We put braces around arrow function bodies if the return value is not significant: () => { this.focused = true; } > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:53 > + this.element.addEventListener("blur", (event) => { I worry that somehow this might miss something, but I didn't have any issues in practice. Created attachment 331971 [details]
Patch for landing
Comment on attachment 331971 [details] Patch for landing Rejecting attachment 331971 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 331971, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/6175003 Created attachment 331972 [details]
Patch for landing
Comment on attachment 331972 [details]
Patch for landing
Forgot to fix the assert.
Created attachment 331974 [details]
Patch for landing
Comment on attachment 331974 [details] Patch for landing Clearing flags on attachment: 331974 Committed r227370: <https://trac.webkit.org/changeset/227370> All reviewed patches have been landed. Closing bug. |