Bug 129034 – isContentEditable shouldn't trigger synchronous style recalc in most cases

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 247316 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/19949863>

[Ryosuke Niwa]

Created attachment 247321 [details]
WIP2

[Ryosuke Niwa]

Created attachment 247323 [details]
Whole WIP2

[Antti Koivisto]

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.

[Chris Dumez]

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();

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 247595 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 247596 [details]
Fixes the bug

[WebKit Commit Bot]

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.

[Ryosuke Niwa]

Created attachment 247597 [details]
Updated for ToT

[WebKit Commit Bot]

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.

[Antti Koivisto]

Comment on attachment 247597 [details]
Updated for ToT

r=me

[WebKit Commit Bot]

Comment on attachment 247597 [details]
Updated for ToT

Clearing flags on attachment: 247597

Committed r180867: <http://trac.webkit.org/changeset/180867>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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?

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Rebaselined tests in r180870.