Bug 129034 - isContentEditable shouldn't trigger synchronous style recalc in most cases
Summary: isContentEditable shouldn't trigger synchronous style recalc in most cases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on: 129035 129144 129200 142003 142078
Blocks: 127832
  Show dependency treegraph
 
Reported: 2014-02-19 01:10 PST by Ryosuke Niwa
Modified: 2015-03-01 14:34 PST (History)
11 users (show)

See Also:


Attachments
WIP (19.70 KB, patch)
2015-02-24 23:24 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP2 (4.44 KB, patch)
2015-02-25 01:26 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Whole WIP2 (15.65 KB, patch)
2015-02-25 01:28 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (94.87 KB, patch)
2015-02-28 00:25 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (34.05 KB, patch)
2015-02-28 00:27 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated for ToT (35.27 KB, patch)
2015-02-28 00:44 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2014-02-19 01:10:45 PST
Right now isContentEditable and related functions forces style recalc just to check
the computed value of -wekbit-user-modify but we shouldn't have to on most documents
as -webkit-user-modify CSS property is almost never used in practice.

On documents that don't use -webkit-user-modify, we should be able to simply traverse up
the DOM tree and find the first node with contenteditable content attribute.

We could even have some flag on Node if we wanted to make it super fast.
Comment 1 Ryosuke Niwa 2015-02-24 23:24:28 PST
Created attachment 247316 [details]
WIP
Comment 2 Radar WebKit Bug Importer 2015-02-24 23:25:03 PST
<rdar://problem/19949863>
Comment 3 Ryosuke Niwa 2015-02-25 01:26:25 PST
Created attachment 247321 [details]
WIP2
Comment 4 Ryosuke Niwa 2015-02-25 01:28:02 PST
Created attachment 247323 [details]
Whole WIP2
Comment 5 Antti Koivisto 2015-02-25 05:00:31 PST
Comment on attachment 247323 [details]
Whole WIP2

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

> Source/WebCore/dom/DocumentStyleSheetCollection.cpp:452
> +    for (auto sheet: m_activeAuthorStyleSheets) {

Space before :

> Source/WebCore/dom/Node.cpp:558
> +static inline bool determineEditabilityIndependentOfStyle(const Node& node, bool isEditable)

You probably want bool& isEditable

> Source/WebCore/editing/htmlediting.cpp:156
> +    switch (updateStyle) {
> +    case UpdateStyle:
> +        if (document.needsStyleRecalc() && !document.styleSheetCollection().usesStyleBasedEditability() && editableType == ContentIsEditable)
> +            return node->isEditableIgnoringStyle();
> +        document.updateStyleIfNeeded();
> +        break;

I prefer the common case (==no editability defined in style) to be the function body and exceptions early returns.

No need to call updateStyleIfNeeded if needsStyleRecalc is already tested false.
Comment 6 Chris Dumez 2015-02-25 09:59:16 PST
Comment on attachment 247323 [details]
Whole WIP2

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

>> Source/WebCore/dom/DocumentStyleSheetCollection.cpp:452
>> +    for (auto sheet: m_activeAuthorStyleSheets) {
> 
> Space before :

Also auto& to avoid ref-counting churn?

>> Source/WebCore/dom/Node.cpp:558
>> +static inline bool determineEditabilityIndependentOfStyle(const Node& node, bool isEditable)
> 
> You probably want bool& isEditable

Good catch :)

> Source/WebCore/dom/Node.cpp:590
> +        if (node->isElementNode())

is<Element>(*node)

> Source/WebCore/dom/Node.cpp:591
> +            return downcast<Element>(node)->matchesReadWritePseudoClass();

for consistency: return downcast<Element>(*node).matchesReadWritePseudoClass();
Comment 7 Ryosuke Niwa 2015-02-27 17:11:34 PST
Comment on attachment 247323 [details]
Whole WIP2

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

>> Source/WebCore/editing/htmlediting.cpp:156
>> +        break;
> 
> I prefer the common case (==no editability defined in style) to be the function body and exceptions early returns.
> 
> No need to call updateStyleIfNeeded if needsStyleRecalc is already tested false.

Even in the case editability is not affected by style, we would still like to use render style to compute the editablity if the style tree is up to date for a better performance so I'd expect the common case to be the one that uses the render tree.
Comment 8 Ryosuke Niwa 2015-02-28 00:25:00 PST
Created attachment 247595 [details]
Fixes the bug
Comment 9 Ryosuke Niwa 2015-02-28 00:27:32 PST
Created attachment 247596 [details]
Fixes the bug
Comment 10 WebKit Commit Bot 2015-02-28 00:29:30 PST
Attachment 247596 [details] did not pass style-queue:


ERROR: Source/WebCore/css/StyleSheetContents.h:164:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Ryosuke Niwa 2015-02-28 00:44:44 PST
Created attachment 247597 [details]
Updated for ToT
Comment 12 WebKit Commit Bot 2015-02-28 00:46:38 PST
Attachment 247597 [details] did not pass style-queue:


ERROR: Source/WebCore/css/StyleSheetContents.h:164:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Antti Koivisto 2015-03-01 10:25:37 PST
Comment on attachment 247597 [details]
Updated for ToT

r=me
Comment 14 WebKit Commit Bot 2015-03-01 11:52:42 PST
Comment on attachment 247597 [details]
Updated for ToT

Clearing flags on attachment: 247597

Committed r180867: <http://trac.webkit.org/changeset/180867>
Comment 15 WebKit Commit Bot 2015-03-01 11:52:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Alexey Proskuryakov 2015-03-01 13:14:13 PST
This broke several tests on Windows:

editing/execCommand/5142012-1.html
editing/execCommand/nsresponder-outdent.html
editing/inserting/insert-at-end-02.html

https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r180867%20(50050)/results.html

Perhaps these just need updated results?
Comment 17 Ryosuke Niwa 2015-03-01 14:17:42 PST
(In reply to comment #16)
> This broke several tests on Windows:
> 
> editing/execCommand/5142012-1.html
> editing/execCommand/nsresponder-outdent.html
> editing/inserting/insert-at-end-02.html
> 
> https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/
> r180867%20(50050)/results.html
> 
> Perhaps these just need updated results?

Yes, they need to be rebaselined on all other platforms.  I've been waiting for bots to catch up so that I can do it.
Comment 18 Ryosuke Niwa 2015-03-01 14:34:38 PST
Rebaselined tests in r180870.