setContentEditable with true/false/inherit string is not working properly

• Component: HTML Editing
• Status: RESOLVED FIXED

[Chang Shu]

The current implementation of setContentEditable does not strictly follow the spec.
Layout tests:
set-true.html
set-false.html
set-inherit-parent-true.html
set-inherit-parent-false.html

[Alexey Proskuryakov]

> set-true.html
> set-false.html
> set-inherit-parent-true.html
> set-inherit-parent-false.html

Where can one find these layout tests?

[Chang Shu]

(In reply to comment #1)
> > set-true.html
> > set-false.html
> > set-inherit-parent-true.html
> > set-inherit-parent-false.html
> 
> Where can one find these layout tests?

:) I was asked to create the bugs and put references in the expected test results. These tests are not landed yet. Please see bug 51957.

[Chang Shu]

Created attachment 80246 [details]
fix patch

As we have discussed, calling document()->updateLayoutIgnorePendingStylesheets() in setContentEditable() is not the ultimate solution for resolving the dom-renderstyle sync problem as setAttribute("contenteditable",value) will still fail. But at least, the "contentEditable = value" case is resolved and it won't cause a big performance regression. So I put this patch for review.

[Darin Adler]

Comment on attachment 80246 [details]
fix patch

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

> Source/WebCore/html/HTMLElement.cpp:743
> +    document()->updateLayoutIgnorePendingStylesheets();

This is wrong. The correct place to put a layout update is in the getter, not the setter. If you make a test case that sets the attribute with setAttribute or removeAttribute you will see that the fix is not effective for that case.

[Darin Adler]

(In reply to comment #3)
> As we have discussed, calling document()->updateLayoutIgnorePendingStylesheets() in setContentEditable() is not the ultimate solution for resolving the dom-renderstyle sync problem as setAttribute("contenteditable",value) will still fail. But at least, the "contentEditable = value" case is resolved and it won't cause a big performance regression. So I put this patch for review.

I see your comment now.

I don’t agree that this is a reasonable intermediate step.

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 80246 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80246&action=review
> 
> > Source/WebCore/html/HTMLElement.cpp:743
> > +    document()->updateLayoutIgnorePendingStylesheets();
> 
> This is wrong. The correct place to put a layout update is in the getter, not the setter. If you make a test case that sets the attribute with setAttribute or removeAttribute you will see that the fix is not effective for that case.

However, updating layout in getting creates a security vulnerability.  I think we should resolve the bug 52025 first.

[Darin Adler]

(In reply to comment #6)
> (In reply to comment #4)
> > This is wrong. The correct place to put a layout update is in the getter, not the setter. If you make a test case that sets the attribute with setAttribute or removeAttribute you will see that the fix is not effective for that case.
> 
> However, updating layout in getting creates a security vulnerability. I think we should resolve the bug 52025 first.

If there is a rush to do this and we want to make getting this value from the DOM to work right now, we can separate internal use of isContentEditable from the DOM-visible version, and put the updateLayout function in the DOM version.

[Ryosuke Niwa]

(In reply to comment #7)
> If there is a rush to do this and we want to make getting this value from the DOM to work right now, we can separate internal use of isContentEditable from the DOM-visible version, and put the updateLayout function in the DOM version.

But we do need to update the layout to obtain the correct value, right?  So we'll end up getting differnt values depending on whether script queried values or not and that might lead to some inconsistent state within WebCore.

[Darin Adler]

(In reply to comment #8)
> But we do need to update the layout to obtain the correct value, right?

Yes.

> So we'll end up getting differnt values depending on whether script queried values or not and that might lead to some inconsistent state within WebCore.

That’s already true.

We can get different values depending on whether the script performs an operation that triggers style computation. To give one trivial example, calling offsetLeft on some object will trigger style computation (and layout).

So having isContentEditable be an additional function that gives the correct value won’t necessarily cause new problems.

[Chang Shu]

> > However, updating layout in getting creates a security vulnerability. I think we should resolve the bug 52025 first.
> 
> If there is a rush to do this and we want to make getting this value from the DOM to work right now, we can separate internal use of isContentEditable from the DOM-visible version, and put the updateLayout function in the DOM version.

Bug 52025 has an impact on the live website and I won't think it will be resolved any time soon. I will creat a new patch based on Darin's idea about using a new function.

[Chang Shu]

> I will creat a new patch based on Darin's idea about using a new function.

I just realized there are tons of places calling isContentEditable(defined in Node.h), which I planed to rename to isEditable for internal use and keep isContentEditable (defined in HTMLElement.idl) for js use. Would you guys go for it?

[Chang Shu]

*** Bug 52059 has been marked as a duplicate of this bug. ***

[Darin Adler]

(In reply to comment #11)
> > I will creat a new patch based on Darin's idea about using a new function.
> 
> I just realized there are tons of places calling isContentEditable(defined in Node.h), which I planed to rename to isEditable for internal use and keep isContentEditable (defined in HTMLElement.idl) for js use. Would you guys go for it?

Yes, renaming the existing function is the right way to do this.

But the name should not be different in an arbitrary way. Instead the difference should express the difference between the functions. The old function is faster but requires that renderers and style be up to date, otherwise gives the wrong name. If we had a sense of the future direction that would help us choose the right name. For example, if we think we are changing all call sites, then deprecatedFastIsContentEditable would be one idea for the name. Another possibility is fastIsContentEditable, which at least answers the question of why you’d use it instead of isContentEditable.

The difficulty of naming this is directly tied to the difficulty of correctly calling the existing function!

[Suresh Voruganti]

Removing the error from Qtwebkit2.1.x Nice to have master bug.

We still need to fix the problem in trunk.

[Chang Shu]

Created attachment 87145 [details]
fix patch 2

[Ryosuke Niwa]

Comment on attachment 87145 [details]
fix patch 2

There are many places where we currently call rendererIsEditable, right?  How do we know that we don't need to replace those calls by isContentEditable?

[Chang Shu]

(In reply to comment #16)
> (From update of attachment 87145 [details])
> There are many places where we currently call rendererIsEditable, right?  How do we know that we don't need to replace those calls by isContentEditable?

Since this patch should not cause any regression (because the original code never updates layout), I think it's reasonable that we fix issues in the future when they show up. For places we know we should call isContentEditable, like the ones you have pointed out, we should fix them now. What do you think?

[Ryosuke Niwa]

Comment on attachment 87145 [details]
fix patch 2

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

Thanks for filing the followup bug.

> Source/WebCore/dom/Node.cpp:716
> +    document()->updateLayoutIgnorePendingStylesheets();

Why do we ignore pending stylesheets?  Don't we need to include pending stylesheets as well?

[Chang Shu]

(In reply to comment #18)
> (From update of attachment 87145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87145&action=review
> 
> Thanks for filing the followup bug.
> 
> > Source/WebCore/dom/Node.cpp:716
> > +    document()->updateLayoutIgnorePendingStylesheets();
> 
> Why do we ignore pending stylesheets?  Don't we need to include pending stylesheets as well?

This has to trace back to the discussion in comment #12 of
https://bugs.webkit.org/show_bug.cgi?id=50636.

[Darin Adler]

(In reply to comment #18)
> > Source/WebCore/dom/Node.cpp:716
> > +    document()->updateLayoutIgnorePendingStylesheets();
> 
> Why do we ignore pending stylesheets?  Don't we need to include pending stylesheets as well?

I don’t think the name makes this entirely clear, but the difference between updateLayout and updateLayoutIgnorePendingStylesheets is related to the state early in page loading, where stylesheets are still loading

To avoid a “flash of unstyled content”, the entire page is set to “display: none” until the stylesheets are loaded. If the caller can live with that, getting null for the renderer if called during that early period, then updateLayout is the right function to call. If the caller needs good data for the renderers even if called during that early period and it’s acceptable to have a FOUC, then updateLayoutIgnorePendingStylesheets is the right function to call.

[Chang Shu]

> > Why do we ignore pending stylesheets?  Don't we need to include pending stylesheets as well?
> 
> I don’t think the name makes this entirely clear, but the difference between updateLayout and updateLayoutIgnorePendingStylesheets is related to the state early in page loading, where stylesheets are still loading
> 
> To avoid a “flash of unstyled content”, the entire page is set to “display: none” until the stylesheets are loaded. If the caller can live with that, getting null for the renderer if called during that early period, then updateLayout is the right function to call. If the caller needs good data for the renderers even if called during that early period and it’s acceptable to have a FOUC, then updateLayoutIgnorePendingStylesheets is the right function to call.

Thanks for sharing the knowledge. It seems to me updateLayoutIgnorePendingStylesheets tries the best and is a better choice for us.

[Ryosuke Niwa]

(In reply to comment #20)
> I don’t think the name makes this entirely clear, but the difference between updateLayout and updateLayoutIgnorePendingStylesheets is related to the state early in page loading, where stylesheets are still loading
> 
> To avoid a “flash of unstyled content”, the entire page is set to “display: none” until the stylesheets are loaded. If the caller can live with that, getting null for the renderer if called during that early period, then updateLayout is the right function to call. If the caller needs good data for the renderers even if called during that early period and it’s acceptable to have a FOUC, then updateLayoutIgnorePendingStylesheets is the right function to call.

I see. Thanks for the clarification, Darin!

(In reply to comment #21)
> Thanks for sharing the knowledge. It seems to me updateLayoutIgnorePendingStylesheets tries the best and is a better choice for us.

I agree.

[Ryosuke Niwa]

Comment on attachment 87145 [details]
fix patch 2

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

r=me provided you make the following change to the change log entry.

> Source/WebCore/ChangeLog:10
> +        checking editability: rendererIsEditable and isContentEdiable. The former is a fast path,
> +        which checks the render style of usermodify. The latter updates the layout first to make

In addition to being fast, rendererIsEditable does NOT trigger layout, which is an important thing in some call sites so you should probably mention that although the function implies it.

[Chang Shu]

(In reply to comment #23)
> (From update of attachment 87145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87145&action=review
> 
> r=me provided you make the following change to the change log entry.
> 
> > Source/WebCore/ChangeLog:10
> > +        checking editability: rendererIsEditable and isContentEdiable. The former is a fast path,
> > +        which checks the render style of usermodify. The latter updates the layout first to make
> 
> In addition to being fast, rendererIsEditable does NOT trigger layout, which is an important thing in some call sites so you should probably mention that although the function implies it.

Thanks for the review, Ryosuke. I will rebaseline the patch with change log update and cq+ it myself.

[Chang Shu]

Created attachment 88108 [details]
fix patch 3

review+ already. only updated changelog and rebaselined.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 88108 [details]:

fast/loader/recursive-before-unload-crash.html bug 50880 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 88108 [details]
fix patch 3

Clearing flags on attachment: 88108

Committed r82886: <http://trac.webkit.org/changeset/82886>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82886 might have broken Windows XP Debug (Tests)

[Chang Shu]

*** Bug 50636 has been marked as a duplicate of this bug. ***