Bug 182619 - Web Inspector: Styles: Loses focus when editing a property while page is being loaded
Summary: Web Inspector: Styles: Loses focus when editing a property while page is bein...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
Keywords: InRadar
Depends on:
Reported: 2018-02-08 13:20 PST by Nikita Vasilyev
Modified: 2018-03-20 21:47 PDT (History)
6 users (show)

See Also:

[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

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2018-02-08 13:20:05 PST
Created attachment 333418 [details]
[Animated GIF] Bug

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

While the page is being loaded, focus may get lost from the value field.
Completion popover doesn't hide, exacerbating the problem.

Page loading should not affect focused CSS property.

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.
Comment 1 Radar WebKit Bug Importer 2018-02-08 13:20:49 PST
Comment 2 Nikita Vasilyev 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.


        // We only need to do a rebuild on significant changes. Other changes are handled
        // by the sections and text editors themselves.
        if (!significantChange) {


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.
Comment 3 Nikita Vasilyev 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.
Comment 4 Nikita Vasilyev 2018-03-12 16:00:18 PDT
Created attachment 335657 [details]
Comment 5 Nikita Vasilyev 2018-03-12 16:04:13 PDT
Created attachment 335658 [details]
[Animated GIF] With patch applied
Comment 6 Nikita Vasilyev 2018-03-14 16:18:39 PDT
theverge.com was unreliable as a reduction page, so I made a small page myself:

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
Comment 7 Nikita Vasilyev 2018-03-14 16:29:27 PDT
Created attachment 335807 [details]
Comment 8 Nikita Vasilyev 2018-03-14 17:42:04 PDT
Created attachment 335815 [details]

Fixed a typo.
Comment 9 Matt Baker 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:

let s = document.createElement("style");
s.innerText = "body { color: red; }";
setInterval(() => {
    s.parentNode ? s.remove() : document.head.append(s);
}, 3000);
    <p>Body text alternates between black and red, every 3 seconds.</p>

Steps to Reproduce:
1. Inspect <p> element
2. Start editing the Style Attribute
  => Focus and changes lost
Comment 10 Timothy Hatcher 2018-03-18 15:15:10 PDT
Comment on attachment 335815 [details]

Resetting review based on Matt's comment.
Comment 11 Nikita Vasilyev 2018-03-19 16:13:26 PDT
Comment on attachment 335815 [details]

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!
Comment 12 Matt Baker 2018-03-20 17:31:43 PDT
Comment on attachment 335815 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=335815&action=review


> 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.


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...
Comment 13 Nikita Vasilyev 2018-03-20 18:57:31 PDT
Created attachment 336169 [details]
Comment 14 WebKit Commit Bot 2018-03-20 20:27:24 PDT Comment hidden (obsolete)
Comment 15 WebKit Commit Bot 2018-03-20 20:27:26 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-03-20 21:07:50 PDT Comment hidden (obsolete)
Comment 17 Nikita Vasilyev 2018-03-20 21:11:11 PDT
Created attachment 336174 [details]
Comment 18 WebKit Commit Bot 2018-03-20 21:47:50 PDT
Comment on attachment 336174 [details]

Clearing flags on attachment: 336174

Committed r229787: <https://trac.webkit.org/changeset/229787>
Comment 19 WebKit Commit Bot 2018-03-20 21:47:52 PDT
All reviewed patches have been landed.  Closing bug.