RESOLVED FIXED 182619
Web Inspector: Styles: Loses focus when editing a property while page is being loaded 
https://bugs.webkit.org/show_bug.cgi?id=182619
Summary Web Inspector: Styles: Loses focus when editing a property while page is bein...
Nikita Vasilyev
Reported 2018-02-08 13:20:05 PST
Created attachment 333418 [details] [Animated GIF] Bug Steps: 1. Open Web Inspector 2. Select <body> in Elements tab 3. Navigate to https://www.theverge.com 4. Quickly add a new "font-family" property 5. Press Tab to focus on the value field Actual: While the page is being loaded, focus may get lost from the value field. Completion popover doesn't hide, exacerbating the problem. Expected: Page loading should not affect focused CSS property. Notes: This isn't specific to theverge.com. The Verge takes a while to load, so it makes the bug easier to reproduce. Something during page load causes re-layout of style rules. The completion popover doesn't get properly dismissed.
Attachments
[Animated GIF] Bug (1021.06 KB, image/gif)
2018-02-08 13:20 PST, Nikita Vasilyev 		no flags
Details
Patch (2.01 KB, patch)
2018-03-12 16:00 PDT, Nikita Vasilyev 		no flags
DetailsFormatted DiffDiff
[Animated GIF] With patch applied (565.41 KB, image/gif)
2018-03-12 16:04 PDT, Nikita Vasilyev 		no flags
Details
Patch (2.27 KB, patch)
2018-03-14 16:29 PDT, Nikita Vasilyev 		no flags
DetailsFormatted DiffDiff
Patch (2.27 KB, patch)
2018-03-14 17:42 PDT, Nikita Vasilyev 		mattbaker: review+
DetailsFormatted DiffDiff
Patch (2.26 KB, patch)
2018-03-20 18:57 PDT, Nikita Vasilyev 		nvasilyev: commit-queue+
DetailsFormatted DiffDiff
Archive of layout-test-results from webkit-cq-02 for mac-sierra (1.88 MB, application/zip)
2018-03-20 20:27 PDT, WebKit Commit Bot 		no flags
Details
Patch (2.25 KB, patch)
2018-03-20 21:11 PDT, Nikita Vasilyev 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2018-02-08 13:20:49 PST
<rdar://problem/37363185>
Nikita Vasilyev
Comment 2 2018-02-12 14:16:37 PST
It looks like sections (SpreadsheetCSSStyleDeclarationSection) are getting removed and added again while being edited. Something during page load causes a full re-layout. SpreadsheetRulesStyleDetailsPanel.js: refresh(significantChange) { // We only need to do a rebuild on significant changes. Other changes are handled // by the sections and text editors themselves. if (!significantChange) { super.refresh(significantChange); return; } this.removeAllSubviews(); Perhaps SpreadsheetRulesStyleDetailsPanel should be smarter about re-layouts. P.S. Things that would help to debug this: — Better filtering in Timelines. Timelines are spammed by codemirror.js. I want to exclude all stacktraces that have calls from codemirror.js — View.js stacktraces. I want to a stacktrace of where the layout was scheduled.
Nikita Vasilyev
Comment 3 2018-02-16 17:54:19 PST
This bug has other unpleasant consequences. After losing the focus, CSSStyleDeclaration stays locked. Clicking on the white space does not add a blank property. When focused on a CSS property name or value, CSSStyleDeclaration gets locked. This was added in Bug 179461 - Web Inspector: Styles Redesign: data corruption when updating values quickly. CSSStyleDeclaration gets unlocked on "blur" event. Blur event does NOT fire on a focused element when it detaches from DOM. (Ryosuke said it was changed in WebKit about a year ago.) SpreadsheetCSSStyleDeclarationEditor.prototype.layout rebuilds every CSS property view, even when the properties are unchanged. Not only it's a lot of unnecessary work, rebuilding a focused CSS property makes it lose the focus without firing "blur" event.
Nikita Vasilyev
Comment 4 2018-03-12 16:00:18 PDT
Created attachment 335657 [details] Patch
Nikita Vasilyev
Comment 5 2018-03-12 16:04:13 PDT
Created attachment 335658 [details] [Animated GIF] With patch applied
Nikita Vasilyev
Comment 6 2018-03-14 16:18:39 PDT
theverge.com was unreliable as a reduction page, so I made a small page myself: http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/style-added-removed-via-js.html Steps: 1. Open Web Inspector 2. Select <body> in Elements tab 3. Navigate to http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/style-added-removed-via-js.html 4. Quickly add a "font-family" property to Style Attribute 5. Press Tab to focus on the value field
Nikita Vasilyev
Comment 7 2018-03-14 16:29:27 PDT
Created attachment 335807 [details] Patch
Nikita Vasilyev
Comment 8 2018-03-14 17:42:04 PDT
Created attachment 335815 [details] Patch Fixed a typo.
Matt Baker
Comment 9 2018-03-16 16:15:09 PDT
The focus is still lost (along with uncommitted changes) when an inherited rule changes for the selected element. Test Case: <script> let s = document.createElement("style"); s.innerText = "body { color: red; }"; setInterval(() => { s.parentNode ? s.remove() : document.head.append(s); }, 3000); </script> <body> <p>Body text alternates between black and red, every 3 seconds.</p> </body> Steps to Reproduce: 1. Inspect <p> element 2. Start editing the Style Attribute => Focus and changes lost
Timothy Hatcher
Comment 10 2018-03-18 15:15:10 PDT
Comment on attachment 335815 [details] Patch Resetting review based on Matt's comment.
Nikita Vasilyev
Comment 11 2018-03-19 16:13:26 PDT
Comment on attachment 335815 [details] Patch I filed a separate issue, since it would require a different kind of a fix. https://bugs.webkit.org/show_bug.cgi?id=183755 Web Inspector: Styles: Don't remove or add style sections while CSS property or selector is focused This problem this patch fixes is that adding/removing a stylesheet resets focus, even if NONE of the rules of that added/removed stylesheet matches the inspected element!
Matt Baker
Comment 12 2018-03-20 17:31:43 PDT
Comment on attachment 335815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335815&action=review r=me > Source/WebInspectorUI/ChangeLog:11 > + selection, and completion. I think this could be reworded as: Adding or removing a stylesheet causes SpreadsheetRulesStyleDetailsPanel to refresh, triggering a layout of all SpreadsheetCSSStyleDeclarationSection child views. This resets the focus, selection, and auto-completion state. > Source/WebInspectorUI/ChangeLog:13 > + This patch prevents re-layouts of SpreadsheetCSSStyleDeclarationSection when its CSS properties are being edited. And: This patch prevents SpreadsheetCSSStyleDeclarationSection from performing a layout when a property is being edited. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:65 > + // Don't re-layout all properties when one of them is being edited. Full re-layout resets "re-layout" is awkward. What about: // Prevent layout of properties when one of them is being edited. A full layout resets...
Nikita Vasilyev
Comment 13 2018-03-20 18:57:31 PDT
Created attachment 336169 [details] Patch
WebKit Commit Bot
Comment 14 2018-03-20 20:27:24 PDT Comment hidden (obsolete)
WebKit Commit Bot
Comment 15 2018-03-20 20:27:26 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 16 2018-03-20 21:07:50 PDT Comment hidden (obsolete)
Nikita Vasilyev
Comment 17 2018-03-20 21:11:11 PDT
Created attachment 336174 [details] Patch
WebKit Commit Bot
Comment 18 2018-03-20 21:47:50 PDT
Comment on attachment 336174 [details] Patch Clearing flags on attachment: 336174 Committed r229787: <https://trac.webkit.org/changeset/229787>
WebKit Commit Bot
Comment 19 2018-03-20 21:47:52 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.