Created attachment 378260 [details] Reduction Steps To Reproduce: 1. Inspect attached reduction.html 2. In the style sidebar, hit the + to add a new selector 3. Type “iframe” as the selector 4. Add “outline: 2px solid orange;” Results: Rule is marked as invalid, but applies. <rdar://problem/54530335>
Created attachment 378262 [details] Patch This fixes the bug but it also updates rules that do match selected node twice. I'm still thinking of a better way of fixing this.
Created attachment 378805 [details] Patch
inspector/css/modify-inline-style.html seems to be failing after this change.
Comment on attachment 378805 [details] Patch This broken tests on wk1 and there only. Looking into it.
(In reply to Nikita Vasilyev from comment #4) > Comment on attachment 378805 [details] > Patch > > This broken tests on wk1 and there only. Looking into it. Updating twice (after CSSAgent.setStyleText and on CSSAgent.getMatchedStylesForNode) caused a race condition on WK1. In my next patch, updates only happen once. If it matches the selected DOM node, update on CSSAgent.getMatchedStylesForNode. If it doesn't, update on CSSAgent.setStyleText.
Created attachment 379798 [details] Patch
I don't understand what is meant by "rule is marked invalid, but applies". 1. Inspect test case 2. Open Elements tab (<foo> is selected by default) 2. Press "+" to add new selector 3. Adds selector: foo { } 4. Type "frame", replacing the selection "foo" 5. Press tab. Type "outline". Press tab. Type "2px solid orange". Press tab. Result: Rule marked invalid, but style doesn't apply (there are no iframe elements in document) Repeating the above without changing "foo" to "iframe" works as expected. Am I missing something?
(In reply to Matt Baker from comment #7) > I don't understand what is meant by "rule is marked invalid, but applies". Property value (e.g. "2px solid orange") is marked invalid (has a red strikethrough and a warning), but applies. See rdar://problem/54530335 if it's still unclear. (In reply to Matt Baker from comment #7) > Repeating the above without changing "foo" to "iframe" works as expected. Am > I missing something? Yes, repeating the above without changing "foo" to "iframe" works as expected, even before the patch. The bug is specifically about the rules that don't match the selected node.
(In reply to Nikita Vasilyev from comment #8) > (In reply to Matt Baker from comment #7) > > I don't understand what is meant by "rule is marked invalid, but applies". > > Property value (e.g. "2px solid orange") is marked invalid (has a red > strikethrough and a warning), but applies. > > See rdar://problem/54530335 if it's still unclear. > > (In reply to Matt Baker from comment #7) > > Repeating the above without changing "foo" to "iframe" works as expected. Am > > I missing something? > > Yes, repeating the above without changing "foo" to "iframe" works as > expected, even before the patch. > > The bug is specifically about the rules that don't match the selected node. Okay I get it. I think it would have been helpful to have an <iframe> in the test case, and mention this.
I saw some strange results with the patch applied. Test page: <div>I'm a div</div> <p>And I'm a paragraph</p> Steps to Reproduce: 1. Inspect test page 2. Elements > select the <div> 3. Press "+" to add new selector 4. Type "p", replacing "div" 5. Hit tab. Type "outline". Hit tab. Type "20px solid orange". Hit tab. => Selector not applied. Property value shown as invalid. 6. Select the <p> node in the DOM tree => Style sidebar shows selector contents: p { outline: outline: 20; outline: 20p; utline: 20; outline: 20px; tline: 20; outline: 20p; utline: 20; outline: 20px saddlebrown; line: 20; outline: 20p; ... } Looks like a parsing step might be getting stuck in a loop.
The console is full of "_styleSheetTextRange data is invalid" errors: [Error] Assertion Failed: Cannot have negative numbers in TextRange 0:-2...0:9 cloneAndModify (TextRange.js:134) _updateOwnerStyleText (CSSProperty.js:463) _updateStyleText (CSSProperty.js:438) rawValue (CSSProperty.js:257) _handleValueChange (SpreadsheetStyleProperty.js:736) spreadsheetTextFieldDidChange (SpreadsheetStyleProperty.js:348) _handleInput (SpreadsheetTextField.js:370) _handleInput [Error] Assertion Failed: _styleSheetTextRange data is invalid. _updateOwnerStyleText (CSSProperty.js:468) _updateStyleText (CSSProperty.js:438) rawValue (CSSProperty.js:257) _handleValueChange (SpreadsheetStyleProperty.js:736) spreadsheetTextFieldDidChange (SpreadsheetStyleProperty.js:348) _handleInput (SpreadsheetTextField.js:370) _handleInput
Created attachment 379983 [details] Patch The problem with the previous patch was that I updated CSSProperty#styleSheetTextRange but didn't update CSSStyleDeclaration#styleSheetTextRange, which caused data corruption.
Comment on attachment 379983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379983&action=review > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:595 > - let existingStyles = this._stylesMap.get(mapKey); > - if (existingStyles && !style) { > - for (let existingStyle of existingStyles) { > - if (existingStyle.node === node && existingStyle.inherited === inherited) { > - style = existingStyle; > - break; > + if (matchesNode) { > + let existingStyles = this._stylesMap.get(mapKey); > + if (existingStyles && !style) { > + for (let existingStyle of existingStyles) { > + if (existingStyle.node === node && existingStyle.inherited === inherited) { > + style = existingStyle; > + break; > + } > } > } > - } > > - if (style) > - this._stylesMap.add(mapKey, style); > + if (style) > + this._stylesMap.add(mapKey, style); > > - var shorthands = {}; > - for (var i = 0; payload.shorthandEntries && i < payload.shorthandEntries.length; ++i) { > - var shorthand = payload.shorthandEntries[i]; > - shorthands[shorthand.name] = shorthand.value; > + var shorthands = {}; > + for (var i = 0; payload.shorthandEntries && i < payload.shorthandEntries.length; ++i) { > + var shorthand = payload.shorthandEntries[i]; > + shorthands[shorthand.name] = shorthand.value; > + } This seem like a lot of changes but I simply wrapped a chunk of code in `if (matchesNode) { ... }`
Comment on attachment 379983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379983&action=review > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:459 > + // Update validity status of each property for rules that don't match the selected DOM node. I'd either say "`valid` status" or "validity". > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:460 > + // These rules don't get updated by CSSAgent.getMatchedStylesForNode. We haven't been adding the "Agent" to comments like these lately, as the `CSSAgent` is purely a frontend concept. It should just be `CSS.getMatchedStylesForNode` (e.g. "Domain.command"). > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:464 > + const inherited = false; > + const pseudoId = null; > + const matchesNode = false; We don't typically create `const` variables when calling functions that are in the same file (especially "private" functions) as it's easy to just scroll up/down, rather than having to switch files. > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:560 > + _parseStyleDeclarationPayload(payload, node, inherited, pseudoId, type, rule, matchesNode = true) We usually put optional values like this in a bag, and often have the "default" case be `false`. ``` _parseStyleDeclarationPayload(payload, node, type, {inherited, pseudoId, rule, nonMatching} = {}) ``` This way, we can make all the callsites a bit cleaner in that they only have to pass what they actually need. > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:575 > let style = rule ? rule.style : null; I'd imagine that if `nonMatching` is provided, you'd expect a `rule` to be provided. We should assert as such: ``` console.assert(!nonMatching || style); ``` > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:578 > + let existingStyles = this._stylesMap.get(mapKey); We still use `mapKey` below in this function, which adds a newly created `WI.CSSStyleDeclaration` to `_stylesMap`, so we should probably add a `console.assert(!nonMatching)` there. > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:594 > + var shorthands = {}; > + for (var i = 0; payload.shorthandEntries && i < payload.shorthandEntries.length; ++i) { > + var shorthand = payload.shorthandEntries[i]; > + shorthands[shorthand.name] = shorthand.value; Can we just remove this code? It doesn't look like it's used for anything.
Comment on attachment 379983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379983&action=review >> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:459 >> + // Update validity status of each property for rules that don't match the selected DOM node. > > I'd either say "`valid` status" or "validity". Thanks. >> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:460 >> + // These rules don't get updated by CSSAgent.getMatchedStylesForNode. > > We haven't been adding the "Agent" to comments like these lately, as the `CSSAgent` is purely a frontend concept. It should just be `CSS.getMatchedStylesForNode` (e.g. "Domain.command"). `CSSAgent.getMatchedStylesForNode` is something you can search for in this file and find a function call. >> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:560 >> + _parseStyleDeclarationPayload(payload, node, inherited, pseudoId, type, rule, matchesNode = true) > > We usually put optional values like this in a bag, and often have the "default" case be `false`. > ``` > _parseStyleDeclarationPayload(payload, node, type, {inherited, pseudoId, rule, nonMatching} = {}) > ``` > This way, we can make all the callsites a bit cleaner in that they only have to pass what they actually need. I'd prefer to update all _parseStyleDeclarationPayload and _parseRulePayload in a follow-up patch. I prefer to separate refactoring patches from bug fixes. Also, I don't want to use `nonMatching` name because it's a negative. I'd prefer to use `matchesNode` or `matchingNode`.
Created attachment 380046 [details] Patch
Comment on attachment 380046 [details] Patch r=me
Comment on attachment 380046 [details] Patch Clearing flags on attachment: 380046 Committed r250633: <https://trac.webkit.org/changeset/250633>
All reviewed patches have been landed. Closing bug.