Summary: | REGRESSION(r243264): Web Inspector: Style pane doesn't update after toggling CSS class | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | bburg, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2019-09-20 17:15:48 PDT
r243264 removed code that cleared the maps. After r243264, maps don't get cleared. function fetchedMatchedStyles(error, matchedRulesPayload, pseudoElementRulesPayload, inheritedRulesPayload) { @@ -142,20 +188,12 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object pseudoElementRulesPayload = pseudoElementRulesPayload || []; inheritedRulesPayload = inheritedRulesPayload || []; - // Move the current maps to previous. - this._previousRulesMap = this._rulesMap; - this._previousStyleDeclarationsMap = this._styleDeclarationsMap; - - // Clear the current maps. - this._rulesMap = {}; - this._styleDeclarationsMap = {}; Because of this, WI.DOMNodeStyles.Event.Refreshed gets called with significantChange set to false. significantChange can be true only when a style rule is new (and not present in previousStylesMap). The style rules only get updated when WI.DOMNodeStyles.Event.Refreshed is fired with significantChange set to true. Created attachment 379298 [details]
Patch
This fixes the bug and clears the maps as it was before the regression was introduced.
However, I've noticed that the existing (and pretty old) code may have race conditions. `fetchedMatchedStyles` and` fetchedComputedStyle` are called in parallel and they modify the same variables. I'll take a closer look.
> However, I've noticed that the existing (and pretty old) code may have race
> conditions. `fetchedMatchedStyles` and` fetchedComputedStyle` are called in
> parallel and they modify the same variables. I'll take a closer look.
Backend commands, unless they are marked `async`, are serial. So the order of responses should be deterministic.
Comment on attachment 379298 [details]
Patch
Oh, the patch should be good to review then!
Comment on attachment 379298 [details]
Patch
Several tests are failing :(
Created attachment 379419 [details]
Patch
To run on bots. Not for review.
Comment on attachment 379419 [details]
Patch
Setting r? so the bots run the tests. Don't review.
Created attachment 379420 [details]
Patch for bots
Comment on attachment 379420 [details] Patch for bots View in context: https://bugs.webkit.org/attachment.cgi?id=379420&action=review r-, we don't want to just revert these changes, as IIRC they were necessary in order for r243264 to work correctly. (In reply to Nikita Vasilyev from comment #2) > r243264 removed code that cleared the maps. After r243264, maps don't get cleared. > > function fetchedMatchedStyles(error, matchedRulesPayload, pseudoElementRulesPayload, inheritedRulesPayload) > { > @@ -142,20 +188,12 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object > pseudoElementRulesPayload = pseudoElementRulesPayload || []; > inheritedRulesPayload = inheritedRulesPayload || []; > > - // Move the current maps to previous. > - this._previousRulesMap = this._rulesMap; > - this._previousStyleDeclarationsMap = this._styleDeclarationsMap; > - > - // Clear the current maps. > - this._rulesMap = {}; > - this._styleDeclarationsMap = {}; > > Because of this, WI.DOMNodeStyles.Event.Refreshed gets called with significantChange set to false. significantChange can be true only when a style rule is new (and not present in previousStylesMap). > > The style rules only get updated when WI.DOMNodeStyles.Event.Refreshed is fired with significantChange set to true. If that's the case, can we change the logic for how `significantChange` is determined instead? It seems like that is the real issue here, not how (or what) we decide to keep in a map. > Source/WebInspectorUI/ChangeLog:9 > + Revert all changes related to `_styleDeclarationsMap`. Why does this solve the problem? Please add some description either to your investigation or why/how you arrived at this conclusion in the ChangeLog. This wasn’t ready for review. Sorry it wasn’t obvious. Bots don’t run tests if I set “r-“ :/ Created attachment 379579 [details]
Patch for bots, not for review
Created attachment 379611 [details]
Patch
Created attachment 379612 [details]
[Animated GIF] With patch applied
Comment on attachment 379611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379611&action=review This causes a UI regression the Styles panel: 1. inspect <https://devinrousso.com> 2. select the node matching `body > header > nav > ul > li:nth-child(1) > a` in the Elements Tab 3. change the `a.roll` selector to be some garbage (e.g. "asdasdasd") 4. change the `header>nav>ul>li>a.selected` selector to be some garbage (e.g. "zxczxczxc") => all the group "headers" (e.g. "Pseudo-Element ..." or "Inherited From ...") move to the bottom of the styles sidebar I think this has something to do with the fact that `_headerMap` (`WI.SpreadsheetRulesStyleDetailsPanel`) uses the specific `WI.CSSStyleDeclaration` instance as a key to reference the DOM node group "header". > Source/WebInspectorUI/ChangeLog:9 > + r243264 never cleared `_styleMap`. Create and clear `_styleMap` at the same place as it was before r243264. Why does clearing the `_styleMap` fix this issue? Can you explain what's going on here? > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:-161 > - let previousStylesMap = this._stylesMap.copy(); I think this was the only usage of `Multimap.prototype.copy`. If it's this expensive, perhaps we should remove it? (In reply to Devin Rousso from comment #15) > Comment on attachment 379611 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379611&action=review > > This causes a UI regression the Styles panel: [...] Yes, I can reproduce it. (In reply to Devin Rousso from comment #15) > Comment on attachment 379611 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379611&action=review > > This causes a UI regression the Styles panel: > 1. inspect <https://devinrousso.com> > 2. select the node matching `body > header > nav > ul > li:nth-child(1) > a` > in the Elements Tab > 3. change the `a.roll` selector to be some garbage (e.g. "asdasdasd") > 4. change the `header>nav>ul>li>a.selected` selector to be some garbage > (e.g. "zxczxczxc") > => all the group "headers" (e.g. "Pseudo-Element ..." or "Inherited From > ...") move to the bottom of the styles sidebar This causes the UI regression because it fixed the significantChange logic. r243264 introduced a regression in the significantChange logic, so WI.SpreadsheetRulesStyleDetailsPanel#layout wouldn't get called when it should. That was the real reason non-matching rules stayed visible. This code was added in r243264: layout() { ... if (preservedSections.length) { for (let section of oldSections) { if (!preservedSections.includes(section)) this.removeSubview(section); } for (let header of this._headerMap.values()) header.remove(); } It's a dead code. If you go through the steps on ToT WebKit, you can see that WI.SpreadsheetRulesStyleDetailsPanel#layout doesn't even get called on step 3 or 4. When I reverted significantChange logic back to normal, this code started running. Comment on attachment 379611 [details]
Patch
r-, since this causes a regression.
Created attachment 407500 [details] Reduction 2 This also reproduces when JS on a web page changes CSS classes related to the inspected element. (via rdar://67419397) Steps: 1. Open "Reduction 2" 2. Inspect <body> 3. Press "Toggle background" button on the web page Expected: The Styles sidebar should no longer show ".foo" rule. There are two distinct use cases in play here: 1. CSS class added/removed from an element causing the style editor to update. The style editor should update with CSSAgent.getMatchedStylesForNode payload. (Currently broken.) 2. Web developer modified CSS selector in the style editor. The style editor should NOT update — the modified CSS rule should stay visible. Pretty straightforward so far. What happens if the 2nd case is followed by the 1st? This is where it may get complicated. Two different solutions: EASY Show only matching rules. That’s what both Chrome DevTools and Firefox DevTools do. HARD Show both matching rules and non-matching rules. If we do this, how do we reconcile matching and non-matching rules? The existing code in WI.SpreadsheetRulesStyleDetailsPanel attempts to save and restore the index of the non-matching rules (note that this code doesn’t actually work, see Comment 17 above). This would cause rather strange behavior when the matching rules change significantly. I think modifying selector to something that doesn’t match the inspected element is relatively rare. Getting CSS classes added/removed after editing a selector to something that doesn’t match the inspected element must be very-very rare. I think we should go with the easy approach. I have a working patch, and I’m working on the tests. Created attachment 407617 [details]
Patch
DOMNodeStyles.js changes here are the same as in the previous patch.
SpreadsheetRulesStyleDetailsPanel.js changes are new.
(In reply to Nikita Vasilyev from comment #20) > EASY > Show only matching rules. That’s what both Chrome DevTools and Firefox > DevTools do. The most straightforward way of implementing this would be to add a flag like _ignoreNextLayout in SpreadsheetRulesStyleDetailsPanel, that is added after editing a selector. This might have worked, but in my experience introducing a state like this tends to produce more error prone code. I didn't pursue this approach. I've gone a slightly different route of implementing the easy approach (see the patch). Comment on attachment 407617 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407617&action=review > Source/WebInspectorUI/ChangeLog:15 > + the same place as it was before r243264. Nit: explain that if the map isn't cleared, then it is not possible to diff old and new style declarations. > Source/WebInspectorUI/ChangeLog:21 > + exiting exiting layout early. Nit: exiting exiting > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:345 > + _getStyleIds(styles) It's weird to put this method here considering it doesn't use `this`. It should be a static method or added to DOMNodeStyles. And the name should be more descriptive, i.e., used like this styles.authorStyles().map((style) => style.stringId) That said, I don't quite understand why this method exists at all. Is it just a workaround for https://webkit.org/b/110055? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:348 > + // FIXME: <https://webkit.org/b/110055> Web Inspector: User Agent style stylesheet identifier keeps incrementing Not sure why this FIXME is relevant. Can you explain or remove it? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:378 > + console.assert(found, "Could not find style rule of edited selector."); Nit: please include the relevant object(s) as extra arguments to console.assert. Here it would be this._renderedStyleIds and section.style.stringId Comment on attachment 407617 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407617&action=review >> Source/WebInspectorUI/ChangeLog:15 >> + the same place as it was before r243264. > > Nit: explain that if the map isn't cleared, then it is not possible to diff old and new style declarations. Good point. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:345 >> + _getStyleIds(styles) > > It's weird to put this method here considering it doesn't use `this`. It should be a static method or added to DOMNodeStyles. And the name should be more descriptive, i.e., used like this > > styles.authorStyles().map((style) => style.stringId) > > That said, I don't quite understand why this method exists at all. Is it just a workaround for https://webkit.org/b/110055? Yes, it's a workaround for https://webkit.org/b/110055. I check if the style ids have changed on line 227. Unfortunately, the UserAgent style ids increment every time and are unreliable. It's safe to exclude them here because UserAgent styles don't get affected by CSS classes being added/removed. Comment on attachment 407617 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407617&action=review >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:345 >>> + _getStyleIds(styles) >> >> It's weird to put this method here considering it doesn't use `this`. It should be a static method or added to DOMNodeStyles. And the name should be more descriptive, i.e., used like this >> >> styles.authorStyles().map((style) => style.stringId) >> >> That said, I don't quite understand why this method exists at all. Is it just a workaround for https://webkit.org/b/110055? > > Yes, it's a workaround for https://webkit.org/b/110055. I check if the style ids have changed on line 227. Unfortunately, the UserAgent style ids increment every time and are unreliable. It's safe to exclude them here because UserAgent styles don't get affected by CSS classes being added/removed. That isn't entirely accurate. There is UserAgent CSS for media controls that adjusts depending on a class, not to mention rules that may/not apply if other things are changed (e.g. the tag of a node, the `type` of an `input`, etc.) although these may already be considered a `significantChange` for other reasons. Yeah, my patch has an issue with changing <input type=checkbox> to <input type=text> - styles don't update :( I tried it on http://nv.github.io/webkit-inspector-bugs/all-element-types.html. Just curious, what's UserAgent CSS for media controls that adjusts depending on a class? Changing <input type=checkbox> to <input type=text> doesn't update styles and it has been broken long before r243264. To fix that, we really need to fix Bug 110055 - Web Inspector: User Agent style stylesheet identifier keeps incrementing. (In reply to Nikita Vasilyev from comment #26) > Yeah, my patch has an issue with changing <input type=checkbox> to <input type=text> - styles don't update :( > I tried it on http://nv.github.io/webkit-inspector-bugs/all-element-types.html. > > Just curious, what's UserAgent CSS for media controls that adjusts depending on a class? I dunno off the top of my head, but I wouldn't be surprised if you could find a handful of examples in Source/WebCore/modules/modern-media-controls/ Created attachment 407694 [details]
Patch
cq- because there's still no tests.
(In reply to Nikita Vasilyev from comment #27) > Changing <input type=checkbox> to <input type=text> doesn't update styles > and it has been broken long before r243264. To fix that, we really need to > fix Bug 110055 - Web Inspector: User Agent style stylesheet identifier keeps > incrementing. I filed "bug 216040: Web Inspector: Elements: styles don't update for certain cases related to UserAgent styles" for this particular case. Created attachment 407715 [details]
Patch
Comment on attachment 407715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407715&action=review r=me, great work! > LayoutTests/ChangeLog:9 > + Added a test to veryfy that WI.DOMNodeStyles.Event.Refreshed fires with appropriate Nit: verify > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:47 > + this._ignoreLayoutAfterSelectorChange = false; Nit: please rename. We typically name this 'suppress' if it's stopping something from happening. i.e., this._suppressLayoutAfterSelectorChange. Created attachment 407718 [details]
Patch
Comment on attachment 407715 [details] Patch nice UI fix, well done :) > LayoutTests/inspector/css/node-styles-refreshed.html:12 > + test(resolve, reject) { NIT: can we make these tests `async` and use `await` instead? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:45 > + this._renderedStyleIds = null; NIT: not used > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:224 > + let uniqueOrderedStyles = this.nodeStyles.uniqueOrderedStyles; NIT: I think you can undo this move now > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:229 > + if (this._selectedNodeId && this._selectedNodeId === this.nodeStyles.node.id) > + return; Is this still needed? I don't think a selector change will also change the `this.nodeStyles.node.id`. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:366 > + this._ignoreLayoutAfterSelectorChange = true; Rather than ignoring the next `layout`, can we use `cancelLayout` instead? Comment on attachment 407715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407715&action=review >> LayoutTests/inspector/css/node-styles-refreshed.html:12 >> + test(resolve, reject) { > > NIT: can we make these tests `async` and use `await` instead? Where would I use `await` in this test? I'm a bit of a noob here. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:45 >> + this._renderedStyleIds = null; > > NIT: not used Thanks! >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:224 >> + let uniqueOrderedStyles = this.nodeStyles.uniqueOrderedStyles; > > NIT: I think you can undo this move now Yep. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:229 >> + return; > > Is this still needed? I don't think a selector change will also change the `this.nodeStyles.node.id`. Yes. I want to always re-layout the style editor when selecting a different element in the DOM tree outline. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:366 >> + this._ignoreLayoutAfterSelectorChange = true; > > Rather than ignoring the next `layout`, can we use `cancelLayout` instead? Hm, I'm a bit concerned by how much WI.View._cancelScheduledLayoutForView is doing. Comment on attachment 407715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407715&action=review >>> LayoutTests/inspector/css/node-styles-refreshed.html:12 >>> + test(resolve, reject) { >> >> NIT: can we make these tests `async` and use `await` instead? > > Where would I use `await` in this test? I'm a bit of a noob here. Fundamentally, tests are very synchronous in the sense that the output has to match the expectation file exactly, meaning that there is an order to "all the things". `async` and `await` allow you to write asynchronous code in such a way that it appears synchronous, even though it's not. In the case of these tests, because `WI.Object` event listeners fire synchronously, we know that the `Promise` returned from `nodeStyles.refresh()` will resolve _after_ `WI.DOMNodeStyles.Event.Refreshed` is dispatched, meaning we know that the `InspectorTest.expectTrue` will be executed before the `finally`. As such, we can simply change this test to be like so: ``` async test() { nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => { InspectorTest.expectTrue(event.data.significantChange, `Adding '.baz' class should cause a significant change.`); }); InspectorTest.evaluateInPage(`document.body.classList.add("baz")`); await nodeStyles.refresh(); } ``` IMO the majority of asynchronous Web Inspector tests could be written with `async` and be _far_ more readable than if they used `resolve`/`reject`. I'd suggest searching for `async test()` in `LayoutTests/inspector/` (the changes I recently made for r266074 are a good example) and looking at how it's used elsewhere too :) >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:229 >>> + return; >> >> Is this still needed? I don't think a selector change will also change the `this.nodeStyles.node.id`. > > Yes. I want to always re-layout the style editor when selecting a different element in the DOM tree outline. Right, but when will this code actually be used in practice? A `layout` should happen after the selector is changed, so any future `layout` (such as if the node is changed) shouldn't have `this._ignoreLayoutAfterSelectorChange` set anyways. >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:366 >>> + this._ignoreLayoutAfterSelectorChange = true; >> >> Rather than ignoring the next `layout`, can we use `cancelLayout` instead? > > Hm, I'm a bit concerned by how much WI.View._cancelScheduledLayoutForView is doing. It's just reversing what's done in `WI.View._scheduleLayoutForView`. It's possible that `cancelLayout` might not work, but if it does work I think that's preferred to keeping another property. Comment on attachment 407715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407715&action=review >>>> LayoutTests/inspector/css/node-styles-refreshed.html:12 >>>> + test(resolve, reject) { >>> >>> NIT: can we make these tests `async` and use `await` instead? >> >> Where would I use `await` in this test? I'm a bit of a noob here. > > Fundamentally, tests are very synchronous in the sense that the output has to match the expectation file exactly, meaning that there is an order to "all the things". `async` and `await` allow you to write asynchronous code in such a way that it appears synchronous, even though it's not. > > In the case of these tests, because `WI.Object` event listeners fire synchronously, we know that the `Promise` returned from `nodeStyles.refresh()` will resolve _after_ `WI.DOMNodeStyles.Event.Refreshed` is dispatched, meaning we know that the `InspectorTest.expectTrue` will be executed before the `finally`. As such, we can simply change this test to be like so: > ``` > async test() { > nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => { > InspectorTest.expectTrue(event.data.significantChange, `Adding '.baz' class should cause a significant change.`); > }); > > InspectorTest.evaluateInPage(`document.body.classList.add("baz")`); > > await nodeStyles.refresh(); > } > ``` > IMO the majority of asynchronous Web Inspector tests could be written with `async` and be _far_ more readable than if they used `resolve`/`reject`. I'd suggest searching for `async test()` in `LayoutTests/inspector/` (the changes I recently made for r266074 are a good example) and looking at how it's used elsewhere too :) Neat! I like it. >>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:229 >>>> + return; >>> >>> Is this still needed? I don't think a selector change will also change the `this.nodeStyles.node.id`. >> >> Yes. I want to always re-layout the style editor when selecting a different element in the DOM tree outline. > > Right, but when will this code actually be used in practice? A `layout` should happen after the selector is changed, so any future `layout` (such as if the node is changed) shouldn't have `this._ignoreLayoutAfterSelectorChange` set anyways. Oh, I see what you're saying. This extra precaution would make sense if there were cases when `layout` doesn't happen after editing a selector. I haven't found such cases so it should be safe to remove it. Created attachment 407741 [details]
Patch
Comment on attachment 407715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407715&action=review >>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:366 >>>> + this._ignoreLayoutAfterSelectorChange = true; >>> >>> Rather than ignoring the next `layout`, can we use `cancelLayout` instead? >> >> Hm, I'm a bit concerned by how much WI.View._cancelScheduledLayoutForView is doing. > > It's just reversing what's done in `WI.View._scheduleLayoutForView`. It's possible that `cancelLayout` might not work, but if it does work I think that's preferred to keeping another property. `cancelLayout` doesn't work here because it cancels a scheduled layout. Here, the layout hasn't been scheduled yet. Committed r266451: <https://trac.webkit.org/changeset/266451> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407741 [details]. |