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...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-08 13:20 PST by Nikita Vasilyev
Modified: 2018-03-20 21:47 PDT (History)
6 users (show)

See Also:


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

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

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.
Comment 1 Radar WebKit Bug Importer 2018-02-08 13:20:49 PST
<rdar://problem/37363185>
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.

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

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:

<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
Comment 10 Timothy Hatcher 2018-03-18 15:15:10 PDT
Comment on attachment 335815 [details]
Patch

Resetting review based on Matt's comment.
Comment 11 Nikita Vasilyev 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!
Comment 12 Matt Baker 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...
Comment 13 Nikita Vasilyev 2018-03-20 18:57:31 PDT
Created attachment 336169 [details]
Patch
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]
Patch
Comment 18 WebKit Commit Bot 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>
Comment 19 WebKit Commit Bot 2018-03-20 21:47:52 PDT
All reviewed patches have been landed.  Closing bug.