Bug 182619

Summary:

Web Inspector: Styles: Loses focus when editing a property while page is being loaded

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Nikita Vasilyev <nvasilyev>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, ews-watchlist, inspector-bugzilla-changes, mattbaker, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

See Also:

https://bugs.webkit.org/show_bug.cgi?id=183755

Attachments:

Description	Flags
[Animated GIF] Bug	none
Patch	none
[Animated GIF] With patch applied	none
Patch	none
Patch	mattbaker: review+
Patch	nvasilyev: commit-queue+
Archive of layout-test-results from webkit-cq-02 for mac-sierra	none
Patch	none

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	Details Formatted Diff Diff
[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	Details Formatted Diff Diff
Patch (2.27 KB, patch) 2018-03-14 17:42 PDT, Nikita Vasilyev	mattbaker: review+	Details Formatted Diff Diff
Patch (2.26 KB, patch) 2018-03-20 18:57 PDT, Nikita Vasilyev	nvasilyev: commit-queue+	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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)

Comment on attachment 336169 [details] Patch Rejecting attachment 336169 [details] from commit-queue. Number of test failures exceeded the failure limit. Full output: http://webkit-queues.webkit.org/results/7046138

WebKit Commit Bot

Comment 15 2018-03-20 20:27:26 PDT Comment hidden (obsolete)

Created attachment 336173 [details] Archive of layout-test-results from webkit-cq-02 for mac-sierra The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-02 Port: mac-sierra Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 16 2018-03-20 21:07:50 PDT Comment hidden (obsolete)

Comment on attachment 336169 [details] Patch Rejecting attachment 336169 [details] from review queue. nvasilyev@apple.com does not have reviewer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.

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.