Bug 50636

Created attachment 75832 [details] fix patch

Attachment 75832 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 75832 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=75832&action=review > WebCore/html/HTMLElement.cpp:701 > + addCSSProperty(attr, CSSPropertyWebkitUserModify, CSSValueInherit); Why set to “inherit” instead of just removing the property?

Comment on attachment 75832 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=75832&action=review > LayoutTests/editing/contentEditable/script-tests/attrInvalidString.js:3 > +shouldBe('document.getElementById("myDiv").contentEditable', '"inherit"'); This test case doesn’t seem to be covering enough. Is this really the only visible difference from that code change?

(In reply to comment #3) > (From update of attachment 75832 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75832&action=review > > > WebCore/html/HTMLElement.cpp:701 > > + addCSSProperty(attr, CSSPropertyWebkitUserModify, CSSValueInherit); > > Why set to “inherit” instead of just removing the property? Right. :) I realized this, too, while working on other cases. I also noticed there seems to be a problem to use renderer()->style()->userModify(). The style is set after the whole script execution is finished instead of taking effect immediately. As a result, if we set contentEditable="true" from "false" and check if (contentEditable=="true") in the next statement, it will fail. Please see the code beblow from ScriptControllerBase.cpp (setUserModify happens inside updateStyleForAllDocuments()). ScriptValue ScriptController::executeScript(const ScriptSourceCode& sourceCode, ShouldAllowXSS shouldAllowXSS) { if (!canExecuteScripts(AboutToExecuteScript) || isPaused()) return ScriptValue(); bool wasInExecuteScript = m_inExecuteScript; m_inExecuteScript = true; ScriptValue result = evaluate(sourceCode, shouldAllowXSS); if (!wasInExecuteScript) { m_inExecuteScript = false; Document::updateStyleForAllDocuments(); } return result; }

(In reply to comment #3) > (From update of attachment 75832 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75832&action=review > > > WebCore/html/HTMLElement.cpp:701 > > + addCSSProperty(attr, CSSPropertyWebkitUserModify, CSSValueInherit); > > Why set to “inherit” instead of just removing the property? Hi, Darin, are you suggesting removing the contentEditable attribute or the CSSPropertyWebkitUserModify property? The removeAttribute approach will work. However, I cannot really remove the usermodify property. It causes an assert in debug mode. I guess the property doesn't exist. But if we do nothing, the default value of renderer()->style()->userModify() turns to be READ_ONLY. I will submit a patch with removeAttribute code. Sounds good?

Created attachment 76204 [details] fix patch 2

(In reply to comment #6) > Hi, Darin, are you suggesting removing the contentEditable attribute or the CSSPropertyWebkitUserModify property? The removeAttribute approach will work. However, I cannot really remove the usermodify property. It causes an assert in debug mode. I guess the property doesn't exist. But if we do nothing, the default value of renderer()->style()->userModify() turns to be READ_ONLY. > I will submit a patch with removeAttribute code. Sounds good? It sounds good but I’m not sure. Test cases will tell us if we’re doing it right. We just have to make sure to test all the detectable side effects after setting contentEditable with each technique and in both cases for inheritance. I think there are three things we can detect 1) the value of the attribute, 2) the value of various style properties in computed style, 3) whether the content is actually editable.

Created attachment 76233 [details] fix patch 3 added more test cases based on Darin's comments.

Comment on attachment 76233 [details] fix patch 3 Thanks for adding more test coverage. I still don’t think we have enough. Specifically, if I call setAttribute("contenteditable", "x") and then getAttribute("contenteditable"), what do I get? I suspect the current patch handles that case wrong.

(In reply to comment #10) > (From update of attachment 76233 [details]) > Thanks for adding more test coverage. I still don’t think we have enough. Specifically, if I call setAttribute("contenteditable", "x") and then getAttribute("contenteditable"), what do I get? I suspect the current patch handles that case wrong. Thanks for the review, Darin. I think the current implementation has some architectural issue with setAttribute/getAttribute. See my comments #5. We use renderer()->style()->userModify() to store the state but it's not updated in time. Do you have any suggestions? This is a separate issue though (e.g. failing to set false from true).

(In reply to comment #11) > We use renderer()->style()->userModify() to store the state but it's not updated in time. Any DOM function that reads a value from style has to call a function to make sure style is updated. If reading the contenteditable style requires querying renderer()->style() then it needs to do what functions like Element::offsetLeft do: document()->updateLayoutIgnorePendingStylesheets(); CSSComputedStyleDeclaration does the same thing. On the other hand, this may be an architecture problem in WebKit. Handling of contenteditable in the style system rather than the DOM may be something we want to eliminate longer term.

Created attachment 76536 [details] fix patch 4 1. added document()->updateLayoutIgnorePendingStylesheets() to force render style update 2. added more test cases that would have failed without this patch.

Hi, Darin, It seems calling removeAttribute() inside setContentEditable() while "Attribute* attr" is still alive will cause memory corruption. But I haven't figured out a workaround.

(In reply to comment #14) > It seems calling removeAttribute() inside setContentEditable() while "Attribute* attr" is still alive will cause memory corruption. But I haven't figured out a workaround. I don’t think we want to actually remove the attribute. Sorry I don’t have time right now to outline the fix. Generally speaking, the DOM attributes can be directly set by code and what we control is just the interpretation of DOM attribute values.

Comment on attachment 76536 [details] fix patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=76536&action=review > WebCore/html/HTMLElement.cpp:651 > + document()->updateLayoutIgnorePendingStylesheets(); > + We definitely don't want to add updateLayout here. We just got rid of this a couple of months ago. > WebCore/html/HTMLElement.cpp:679 > + > + document()->updateLayoutIgnorePendingStylesheets(); > + Why do we want to call updateLayout here? > WebCore/html/HTMLElement.cpp:712 > + } else { // Set default value to "inherit". > + ExceptionCode ec; > + removeAttribute(contenteditableAttr, ec); As Darin suggested, I don't think we should be removing the attribute.

(In reply to comment #16) > (From update of attachment 76536 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76536&action=review > > > WebCore/html/HTMLElement.cpp:651 > > + document()->updateLayoutIgnorePendingStylesheets(); > > + > > We definitely don't want to add updateLayout here. We just got rid of this a couple of months ago. I'm talking about http://trac.webkit.org/changeset/65681

> I don’t think we want to actually remove the attribute. Sorry I don’t have time right now to outline the fix. Generally speaking, the DOM attributes can be directly set by code and what we control is just the interpretation of DOM attribute values. Darin, do you suggest to add a new field (2 bits) in NodeRareData?

(In reply to comment #18) > Darin, do you suggest to add a new field (2 bits) in NodeRareData? No, I don’t think we need that. Here is my analysis: 1) The HTMLElement::setContentEditable(Attribute*) function needs to be renamed. It is not a setContentEditable function. It's a parseContentEditableAttribute function and that's what it should be named. 2) The HTMLElement contentEditable DOM attribute. Unless the HTML5 specification or compatibility problems say otherwise, this should be a reflection of the contenteditable content attribute, what you get with the getAttribute function. It shouldn't process the string you get or set in any way. To implement this, HTMLElement::contentEditable and HTMLElement::setContentEditable should both be removed and instead we should just use [Reflect] in the IDL file. We should test to see if we can do this without breaking the behavior of the rest of the code. This will require changes to the parseContentEditableAttribute function; today that function will do nothing if the contenteditable attribute has an unrecognized value, but that is incorrect. 3) HTMLElement::isContentEditable and HTMLElement::isContentRichlyEditable. These functions are renderer queries and should be moved off of HTMLElement and into the render tree classes. Callers should be modified to call the render tree to get these answers and not ask the DOM element. This could be tricky for functions like HTMLElement::supportsFocus that are in the DOM but want to ask a render tree question. Those too might need to be moved to the render tree, and that in turn could cause us some major headaches, but the problem is already there, just hidden. 4) Removing the ability to control editability with style. Hyatt has proposed that we eliminate the -webkit-user-modify CSS style. If we do that we could move back the editability checks from the render tree to the DOM tree. Item (3) above is hard, so we might need to consider (4), but this removes a feature that some WebKit-specific websites may depend on. I don’t know which of the items above are needed to resolve this bug. And maybe more than the above is needed. I think a good starting point would be to create and land the test cases with expected failures before we start doing the work above.

> Here is my analysis: > > 1) The HTMLElement::setContentEditable(Attribute*) function needs to be renamed. It is not a setContentEditable function. It's a parseContentEditableAttribute function and that's what it should be named. > > 2) The HTMLElement contentEditable DOM attribute. Unless the HTML5 specification or compatibility problems say otherwise, this should be a reflection of the contenteditable content attribute, what you get with the getAttribute function. It shouldn't process the string you get or set in any way. To implement this, HTMLElement::contentEditable and HTMLElement::setContentEditable should both be removed and instead we should just use [Reflect] in the IDL file. We should test to see if we can do this without breaking the behavior of the rest of the code. This will require changes to the parseContentEditableAttribute function; today that function will do nothing if the contenteditable attribute has an unrecognized value, but that is incorrect. > > 3) HTMLElement::isContentEditable and HTMLElement::isContentRichlyEditable. These functions are renderer queries and should be moved off of HTMLElement and into the render tree classes. Callers should be modified to call the render tree to get these answers and not ask the DOM element. This could be tricky for functions like HTMLElement::supportsFocus that are in the DOM but want to ask a render tree question. Those too might need to be moved to the render tree, and that in turn could cause us some major headaches, but the problem is already there, just hidden. > > 4) Removing the ability to control editability with style. Hyatt has proposed that we eliminate the -webkit-user-modify CSS style. If we do that we could move back the editability checks from the render tree to the DOM tree. Item (3) above is hard, so we might need to consider (4), but this removes a feature that some WebKit-specific websites may depend on. > > I don’t know which of the items above are needed to resolve this bug. And maybe more than the above is needed. > > I think a good starting point would be to create and land the test cases with expected failures before we start doing the work above. Thanks for the details, Darin. To work on the test cases first, can you take a look at my latest patch above? I put quite some test cases there but likely something is still missing.

> > 2) The HTMLElement contentEditable DOM attribute. Unless the HTML5 specification or compatibility problems say otherwise, this should be a reflection of the contenteditable content attribute, what you get with the getAttribute function. It shouldn't process the string you get or set in any way. To implement this, HTMLElement::contentEditable and HTMLElement::setContentEditable should both be removed and instead we should just use [Reflect] in the IDL file. We should test to see if we can do this without breaking the behavior of the rest of the code. This will require changes to the parseContentEditableAttribute function; today that function will do nothing if the contenteditable attribute has an unrecognized value, but that is incorrect. I tried the [Reflect] and it's basically working. However, the call to elem.contentEditable should return "inherit" even if elem has no contentEditble attribute. Would [Reflect] break this? Is there a way I can set default value?

(In reply to comment #21) > However, the call to elem.contentEditable should return "inherit" even if elem has no contentEditble attribute. Why? Is this something covered by the HTML5 specification? Is this just to match the other browsers? It's easy enough to implement that rule; we will have to hand-write the function instead of just using [Reflect].

(In reply to comment #22) > (In reply to comment #21) > > However, the call to elem.contentEditable should return "inherit" even if elem has no contentEditble attribute. > > Why? Is this something covered by the HTML5 specification? Is this just to match the other browsers? > > It's easy enough to implement that rule; we will have to hand-write the function instead of just using [Reflect]. I see now in HTML5. I was wrong, it’s not really a reflection!

The HTML5 standard makes it clear. Setting to "inherit" means removing the attribute. Setting to "true" or "false" sets the attribute. That's what seContentEditable needs to do. The getter needs the exact same logic as the setter.

The HTMLElement::contentEditable getter needs to read and parse the contenteditable attribute value. It should not look at the renderer. The HTMLElement::setContentEditable needs to follow the specification rather than what it currently does.

To sync up renderer property(userModify) with DOM attribute (contentEditable), it seems I have to bring back the INHERIT enum in userModify. Does it sound ok? I am working on a patch that will be similar to the 1st patch but function contentEditable and setContentEditable will follow the spec and only deal with DOM attribute.

(In reply to comment #26) > To sync up renderer property(userModify) with DOM attribute (contentEditable), it seems I have to bring back the INHERIT enum in userModify. Does it sound ok? No. I don’t think so. Resolving style also resolves the inheritance. The userModify value would have a value of inherit, it would have the actual inherited value.

I meant to say "userModify would not have a value of inherit, it would have the actual inherited value".

Fix required for Qtwebkit 2.2 release.

(In reply to comment #27) > (In reply to comment #26) > > To sync up renderer property(userModify) with DOM attribute (contentEditable), it seems I have to bring back the INHERIT enum in userModify. Does it sound ok? > > No. I don’t think so. Resolving style also resolves the inheritance. The userModify value would have a value of inherit, it would have the actual inherited value. Sure. In isContentEditable(), if I check userModify, there's an update issue as we discussed before; and if I check DOM attribute, maybe there's performance concern. Any inputs? Btw, how do my test cases look like in bug 51957? Thanks!

(In reply to comment #30) > In isContentEditable(), if I check userModify, there's an update issue as we discussed before The isContentEditable function has to check userModify. Otherwise the CSS style -webkit-usermodify property won't work any more. We can only change it to not check userModify if we first remove that feature. > if I check DOM attribute, maybe there's performance concern No, I don’t think there’s a performance concern. We should be able to check a DOM attribute value with acceptable performance. > Btw, how do my test cases look like in bug 51957? Sorry, I have not had a chance yet.

> > WebCore/html/HTMLElement.cpp:651 > > + document()->updateLayoutIgnorePendingStylesheets(); > > + > > We definitely don't want to add updateLayout here. We just got rid of this a couple of months ago. > > > WebCore/html/HTMLElement.cpp:679 > > + > > + document()->updateLayoutIgnorePendingStylesheets(); > > + > > Why do we want to call updateLayout here? > The problem is the value of userModify is not updated as a blocking call inside setContentEditable. It is updated in the next rederstyleupdate. This makes the isContentEditable js call fail if it is the next statement in js. My next patch is almost ready except a solution for this issue.

(In reply to comment #32) > > > WebCore/html/HTMLElement.cpp:651 > > > + document()->updateLayoutIgnorePendingStylesheets(); > > > We definitely don't want to add updateLayout here. We just got rid of this a couple of months ago. If the subsequent code accesses renderers, we can't just remove updateLayout and then access renderers. No matter when it was done and why, that’s going to result in bugs and crashes. So we may have a problem to solve. If we can’t add the updateLayout then we have remove the access to the renderers (including RenderStyle). > > > WebCore/html/HTMLElement.cpp:679 > > > + document()->updateLayoutIgnorePendingStylesheets(); > > > > Why do we want to call updateLayout here? > > The problem is the value of userModify is not updated as a blocking call inside setContentEditable. It is updated in the next rederstyleupdate. This makes the isContentEditable js call fail if it is the next statement in js. My next patch is almost ready except a solution for this issue. I’ll say it stronger than that. We can’t safely access renderers from an incoming DOM call without first calling updateLayout.

(In reply to comment #33) > If the subsequent code accesses renderers, we can't just remove updateLayout and then access renderers. No matter when it was done and why, that’s going to result in bugs and crashes. So we may have a problem to solve. If we can’t add the updateLayout then we have remove the access to the renderers (including RenderStyle). We talked about this on IRC and it turned out that we can call updateLayout in setContentEditable. I think updating layout in setter is safer than doing it in accessor. We may slightly regress the performance but I don't think many websites modify the value of contenteditable property frequently and repeatedly in a performance critical path. > > The problem is the value of userModify is not updated as a blocking call inside setContentEditable. It is updated in the next rederstyleupdate. This makes the isContentEditable js call fail if it is the next statement in js. My next patch is almost ready except a solution for this issue. > > I’ll say it stronger than that. We can’t safely access renderers from an incoming DOM call without first calling updateLayout. Having said that, it's true that we call updateLayout in other accessors. The problem seems to be that we're calling Element::contentEditable when we shouldn't. smfr might know why need to make such calls.

(In reply to comment #34) > (In reply to comment #33) > > If the subsequent code accesses renderers, we can't just remove updateLayout and then access renderers. No matter when it was done and why, that’s going to result in bugs and crashes. So we may have a problem to solve. If we can’t add the updateLayout then we have remove the access to the renderers (including RenderStyle). > > We talked about this on IRC and it turned out that we can call updateLayout in setContentEditable. You may have talked about it, but you are wrong! The JavaScript could have called o.setAttribute("contenteditable", x) rather than calling o.contentEditable = x. Adding a call to updateLayout in setContentEditable does no good. > I think updating layout in setter is safer than doing it in accessor. Perhaps true, but irrelevant. > Having said that, it's true that we call updateLayout in other accessors. That's right. Because we have to. > The problem seems to be that we're calling Element::contentEditable when we shouldn't. smfr might know why need to make such calls. You mean Element::isContentEditable. Maybe. One problem is that a basic functional question like whether we can edit in a particular area of the DOM is not something that should depend on a style calculation. Another is that internal calls might have reason to know layout is already up to date, and so perhaps shouldn't share a code that can be called by arbitrary JavaScript through the DOM. Hyatt’s suggestion, as I mentioned before, was to make it so that CSS style could not make something editable. If we do that it might make this easier to fix.

(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #33) > > > If the subsequent code accesses renderers, we can't just remove updateLayout and then access renderers. No matter when it was done and why, that’s going to result in bugs and crashes. So we may have a problem to solve. If we can’t add the updateLayout then we have remove the access to the renderers (including RenderStyle). > > > > We talked about this on IRC and it turned out that we can call updateLayout in setContentEditable. > > You may have talked about it, but you are wrong! > > The JavaScript could have called o.setAttribute("contenteditable", x) rather than calling o.contentEditable = x. Adding a call to updateLayout in setContentEditable does no good. In addition, there are other types of changes that could make it a problem. The DOM code could change a property that causes renderer to be destroyed and then one that should cause it to be recreated. Such as an element that had contentEditable set to true while it has display property of "none" and then had that property removed. If we then call isContentEditable we will get false, which is wrong.

(In reply to comment #35) > One problem is that a basic functional question like whether we can edit in a particular area of the DOM is not something that should depend on a style calculation. Another is that internal calls might have reason to know layout is already up to date, and so perhaps shouldn't share a code that can be called by arbitrary JavaScript through the DOM. > > Hyatt’s suggestion, as I mentioned before, was to make it so that CSS style could not make something editable. If we do that it might make this easier to fix. i.e. getting rid of -webkit-user-modify? I'm happy with that approach but I'm afraid that there are some tests and websites that depend on it.

(In reply to comment #37) > (In reply to comment #35) > > Hyatt’s suggestion, as I mentioned before, was to make it so that CSS style could not make something editable. If we do that it might make this easier to fix. > > i.e. getting rid of -webkit-user-modify? I'm happy with that approach but I'm afraid that there are some tests and websites that depend on it. Yes, it’s more of a long term solution than a short term one. You could talk to Hyatt about that. Until or unless we make that change, we’re going to have to wean the editing code off calls to isContentEditable when renderers and style is not up to date. I don’t think there’s a shortcut here.

Removing from Qtwebkit 2.1.x nice to have master bug.

52025 no longer blocks this bug anymore. An alternative approach is found and landed in several patches, mainly in bug 52058. This bug can be closed.

*** This bug has been marked as a duplicate of bug 52058 ***