<rdar://problem/47917373>
Created attachment 361965 [details] Patch
Created attachment 361971 [details] [Video] Bug The problem was mismatching text ranges, which caused all kind of data corruption bugs.
Created attachment 361972 [details] Patch I removed unrelated code changes.
Comment on attachment 361972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361972&action=review rs=me, please wait for EWS > LayoutTests/inspector/css/modify-inline-style.html:7 > + let nodeStyles = null; Style: missing newline after. > LayoutTests/inspector/css/modify-inline-style.html:11 > + name: "AddPropertyAndEdit", Please prefix the test case name with the test suite name. name: "ModifyInlineStyle.AddPropertyAndEdit", > LayoutTests/inspector/css/modify-inline-style.html:27 > + fontProperty.rawValue = "12px normal sans-serif!important"; Style: missing space before `!important`. > LayoutTests/inspector/css/modify-inline-style.html:28 > + let colorProperty = null; Style: missing newline before. > LayoutTests/inspector/css/modify-inline-style.html:30 > + WI.CSSProperty.addEventListener(WI.CSSProperty.Event.Changed, function(event) { NIT: just in case, you should remove this listener when the test case completes. NIT: arrow function? > LayoutTests/inspector/css/modify-inline-style.html:38 > + fontProperty.awaitEvent(WI.CSSProperty.Event.Changed).then((event) => { Please put the `.then(` on a new line. > LayoutTests/inspector/css/modify-inline-style.html:56 > + }).then(() => { > + resolve(); > + return true; > + }); Rather than put `resolve()` inside a `.then(`, you can use it directly as the callback. .then(resolve, reject); > Source/WebInspectorUI/ChangeLog:10 > + the actual text of the payload â it has an extra empty line at the end. non-ascii character :( > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:539 > + if (payload.range) { In order to avoid retyping `payload.range` everywhere, you should pull it out into a variable (like all the other ones at the top of this function) and include a fallback value (an empty object). > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:540 > + // Last property of inline style has mismatching range. If this is only supposed to happen for inline styles, we should assert inside the last `if` that `styleDeclaration.type === WI.CSSStyleDeclaration.Type.Inline`. If not, please remove this comment (or make it more general). > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:544 > + let textLineCount = payload.text.lineCount; You can just use `text`, since it has a fallback as well. > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:547 > + payload.range.endColumn = payload.range.startColumn + payload.text.lastLine.length; Ditto (>544).
Comment on attachment 361972 [details] Patch Thanks for the review! View in context: https://bugs.webkit.org/attachment.cgi?id=361972&action=review >> LayoutTests/inspector/css/modify-inline-style.html:27 >> + fontProperty.rawValue = "12px normal sans-serif!important"; > > Style: missing space before `!important`. I think we should use all kind of code styles for test data. The no-space !important is used on medium.com. >> Source/WebInspectorUI/ChangeLog:10 >> + the actual text of the payload â it has an extra empty line at the end. > > non-ascii character :( Our review tools are bad :(
Created attachment 361984 [details] Patch
Comment on attachment 361984 [details] Patch Unrelated test failures on Mac Debug. cq+
Comment on attachment 361984 [details] Patch Clearing flags on attachment: 361984 Committed r241497: <https://trac.webkit.org/changeset/241497>
All reviewed patches have been landed. Closing bug.
The new test inspector/css/modify-inline-style.html added in https://trac.webkit.org/changeset/241497/webkit is a constant timeout on Mac Release WK2. History: http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcss%2Fmodify-inline-style.html
Re-opened since this is blocked by bug 194676
Created attachment 362101 [details] Patch
Comment on attachment 362101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362101&action=review > LayoutTests/inspector/css/modify-inline-style.html:85 > + nodeStyles.addEventListener(WI.DOMNodeStyles.Event.NeedsRefresh, function(event) { > + // Normally, this would get called from views if the styles sidebar is visible. > + // This doesn't work in tests. > + event.target.refresh(); > + }); Joe and I investigated the problem. This fixed test timeouts on WK2. The rest of the patch is the same as the one that was rolled out.
(In reply to Nikita Vasilyev from comment #13) > Comment on attachment 362101 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362101&action=review > > > LayoutTests/inspector/css/modify-inline-style.html:85 > > + nodeStyles.addEventListener(WI.DOMNodeStyles.Event.NeedsRefresh, function(event) { > > + // Normally, this would get called from views if the styles sidebar is visible. > > + // This doesn't work in tests. > > + event.target.refresh(); > > + }); > > Joe and I investigated the problem. This fixed test timeouts on WK2. The > rest of the patch is the same as the one that was rolled out. Yeah, I really think this is something all of the DOMNodeStyles tests might want to consider. Maybe we should add a TestExtras.js that does something like this, because we are almost certainly going to forgot to do this in other tests!
Comment on attachment 362101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362101&action=review rs=me >>> LayoutTests/inspector/css/modify-inline-style.html:85 >>> + nodeStyles.addEventListener(WI.DOMNodeStyles.Event.NeedsRefresh, function(event) { >>> + // Normally, this would get called from views if the styles sidebar is visible. >>> + // This doesn't work in tests. >>> + event.target.refresh(); >>> + }); >> >> Joe and I investigated the problem. This fixed test timeouts on WK2. The rest of the patch is the same as the one that was rolled out. > > Yeah, I really think this is something all of the DOMNodeStyles tests might want to consider. > > Maybe we should add a TestExtras.js that does something like this, because we are almost certainly going to forgot to do this in other tests! I think you should probably do something like this at the top level: WI.DOMNodeStyles.addEventListener(WI.DOMNodeStyles.Event.NeedsRefresh, (event) => { event.target.refresh(); }); So that any DOMNodeStyles that needs a refresh will refresh. Let me know if that works.
Created attachment 362157 [details] Patch
Yes, it worked.
Comment on attachment 362157 [details] Patch Clearing flags on attachment: 362157 Committed r241623: <https://trac.webkit.org/changeset/241623>
*** Bug 194662 has been marked as a duplicate of this bug. ***