Bug 173232 - Web Inspector: Don't use -webkit-user-modify CSS property
Summary: Web Inspector: Don't use -webkit-user-modify CSS property
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:
Depends on:
Blocks:
 
Reported: 2017-06-10 18:26 PDT by Nikita Vasilyev
Modified: 2017-06-13 14:03 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.28 KB, patch)
2017-06-12 14:33 PDT, Nikita Vasilyev
hi: review+
Details | Formatted Diff | Diff
Patch (4.69 KB, patch)
2017-06-13 13:24 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 2017-06-10 18:26:33 PDT
Replace -webkit-user-modify CSS property with contentEditable HTML attribute.

According to rniwa, -webkit-user-modify CSS property is slow and shouldn't be used.

In addition to this, it has been deprecated in Chrome: https://www.chromestatus.com/feature/4725222129270784
Comment 1 Nikita Vasilyev 2017-06-10 18:29:44 PDT
It is currently used in two places:

Editing.css:
.editing {
    -webkit-user-modify: read-write-plaintext-only;
}

VisualStyleSelectorTreeItem.css:
.item.visual-style-selector-item:not(.dom-element-icon).editable > .titles > .title {
    -webkit-user-modify: read-write-plaintext-only;
}

Both should use contenteditable="plaintext-only" HTML attribute instead.
Comment 2 Nikita Vasilyev 2017-06-12 14:33:44 PDT
Created attachment 312697 [details]
Patch
Comment 3 Devin Rousso 2017-06-12 16:45:31 PDT
Comment on attachment 312697 [details]
Patch

r=me

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

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:153
> +        element.contentEditable = false;

NIT: I think this should be `this.contentEditable = false;` to match the previous line.

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:279
> +            if (container.parentNode.contentEditable) {

NIT: I think that using the `classList.contains("editing")` is more specific than just checking for "contentEditable".  I would leave this as it was.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:242
> +        this._mainTitleElement.contentEditable = this.selected ? "plaintext-only" : false;

NIT: I think this should be moved after the next line to keep consistent ordering between `this._listItemNode` and `this._mainTitleElement`.
Comment 4 Nikita Vasilyev 2017-06-13 13:24:48 PDT
Created attachment 312796 [details]
Patch
Comment 5 WebKit Commit Bot 2017-06-13 14:03:56 PDT
Comment on attachment 312796 [details]
Patch

Clearing flags on attachment: 312796

Committed r218202: <http://trac.webkit.org/changeset/218202>
Comment 6 WebKit Commit Bot 2017-06-13 14:03:58 PDT
All reviewed patches have been landed.  Closing bug.