Bug 53031

Created attachment 79950 [details] fix patch

Comment on attachment 79950 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=79950&action=review > Source/WebCore/html/HTMLElement.cpp:660 > + for (const ContainerNode* p = this; p; p = p->parentNode()) { Please don't one-letter variable name. > Source/WebCore/html/HTMLElement.cpp:667 > + if (p->isHTMLElement()) { > + const HTMLElement* htmlParent = static_cast<const HTMLElement*>(p); > + if (htmlParent->contentEditable() != "inherit") { > + isContentEditableSet = true; > + break; > + } > + } htmlParent is used only once. We don't need to declare this local variable here. Please do: if (p->isHTMLElement() && tatic_cast<const HTMLElement*>(p)->contentEditable() != "inherit") { isCOntentEditableSet = true; break; } Also, it's still very expensive to compare strings here. The last time I checked, isContentEditable was in our hot path, and it's crucial that we keep this function fast. I can't r+ this patch unless we come up with more efficient way of figuring this out.

Comment on attachment 79950 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=79950&action=review I am confused about this patch. Is editability actually wrong, or just the value of the isContentEditable function? I suspect this change is incorrect, but I don’t have enough information to be sure. Also, this change has a high risk of causing a major performance regression. Changing the isContentEditable function to an operation that is O(depth of tree) and that calls the contentEditable function, which allocates a new string object each time it is called, might cause a significant slowdown of many editing operations. > Source/WebCore/html/HTMLElement.cpp:660 > + for (const ContainerNode* p = this; p; p = p->parentNode()) { Please use a word for the name of the local variable, such as parent or node, rather than the letter p. > Source/WebCore/html/HTMLElement.cpp:665 > + isContentEditableSet = true; > + break; If the semantics of this function is correct, then the implementation can be simplified by just doing a return true here. There is no need to use for the boolean local variable. Or we could factor this loop out into a separate helper function.

Comment on attachment 79950 [details] fix patch Restoring the review-.

Thanks so much for the review, Niwa and Darin. Please see my comments for non-trial issues. > I am confused about this patch. Is editability actually wrong, or just the value of the isContentEditable function? I suspect this change is incorrect, but I don’t have enough information to be sure. I think editability was wrong. I ran all editing tests after my change but nothing regressed though. > Also, this change has a high risk of causing a major performance regression. Changing the isContentEditable function to an operation that is O(depth of tree) and that calls the contentEditable function, which allocates a new string object each time it is called, might cause a significant slowdown of many editing operations. > Also, it's still very expensive to compare strings here. The last time I checked, isContentEditable was in our hot path, and it's crucial that we keep this function fast. I can't r+ this patch unless we come up with more efficient way of figuring this out. Right. I had this concern, too. Based on the existing implementation, which uses usermodify, I think when contenteditable is "inherit" along the tree, no usermodify data should be created and the code then checks frame's designmode. I will try if this can be done.

> Thanks so much for the review, Niwa and Darin. Please see my comments for non-trial issues. I mean, non-trivial. :)

(In reply to comment #5) > > I am confused about this patch. Is editability actually wrong, or just the value of the isContentEditable function? I suspect this change is incorrect, but I don’t have enough information to be sure. > > I think editability was wrong. I ran all editing tests after my change but nothing regressed though. If we think we’re fixing a bug, but no test results change, then we have insufficient coverage. We need tests that cover actual editability if that is something being fixed here. > Right. I had this concern, too. Based on the existing implementation, which uses usermodify, I think when contenteditable is "inherit" along the tree, no usermodify data should be created and the code then checks frame's designmode. > I will try if this can be done. Yes, that’s the kind of fix that would fit in with the current implementation much better.

I am thinking about two approaches: 1. Update the default render style userModify (it was read_only initially) to read_write inside Document::setDesignMode(). 2. Use an "inherit" value for usermodify initially. If the code reads usermodify to be "inherit", it continue to check document::inDesignMode(). Actually, I see desingMode itself is a type of InheritedBool. Do they make sense?

(In reply to comment #8) > 1. Update the default render style userModify (it was read_only initially) to read_write inside Document::setDesignMode(). > 2. Use an "inherit" value for usermodify initially. If the code reads usermodify to be "inherit", it continue to check document::inDesignMode(). Actually, I see desingMode itself is a type of InheritedBool. All CSS styles support inheritance. You don’t need to add an inherit value to a style, because that’s just how CSS works. We should ask a CSS expert the right way to have the design mode of a document change the style of the top level element inside the document.

Created attachment 80469 [details] fix patch This patch is work-in-progress. I noticed it caused a regression on editing/deleting/5168598.html. However, I am not sure if it's actually a good thing. --- /tmp/layout-test-results/editing/deleting/5168598-expected.txt 2011-01-28 14:31:13.000000000 -0500 +++ /tmp/layout-test-results/editing/deleting/5168598-actual.txt 2011-01-28 14:31:13.000000000 -0500 @@ -17,5 +17,4 @@ RenderText {#text} at (0,0) size 0x0 layer at (13,83) size 119x13 RenderBlock {DIV} at (3,3) size 119x13 - RenderBR {BR} at (1,0) size 0x13 -caret: position 0 of child 0 {BR} of child 0 {DIV} of child 3 {INPUT} of body +caret: position 0 of child 0 {DIV} of child 3 {INPUT} of body

Created attachment 80675 [details] fix patch 2

Looking at time profiles I posted on https://bugs.webkit.org/show_bug.cgi?id=53649, this patch might improve WebKit's performance a lot.

Comment on attachment 80675 [details] fix patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=80675&action=review > Source/WebCore/html/HTMLElement.cpp:-660 > - if (document()->frame() && document()->frame()->isContentEditable()) > - return true; > - This will eliminate calls to EditorClient's clientIsEditable() method. I think we should per the bug 53649 but if we're removing the call here then we should be consistent across the code base. I used Google codesearch and found that clientIsEditable() is called in exactly two places: 1. Frame::isContentEditable - called by AccessibilityRenderObject::isReadOnly 2. DragController::operationForLoad I think we should get rid of Frame's isContentEditable method entirely and discontinue the support for EditorClient::clientIsEditable() since it's our No.1 performance bottleneck. But you should probably make EditorClient change a separate patch because it's going to be large and will involve lots of ports. I checked several major port's EditorClient but most of clientIsEditable implementation was to return a boolean member variable so removing clientIsEditable is unlikely to break things.

(In reply to comment #12) > Looking at time profiles I posted on https://bugs.webkit.org/show_bug.cgi?id=53649, this patch might improve WebKit's performance a lot. I believe so. isContentEditable is called so frequently that I see it's triggered multiple times on each repaint. Even worse, frame()->isContentEditable() will traverse through the entire branch and most of time, does nothing.

(In reply to comment #14) > I believe so. isContentEditable is called so frequently that I see it's triggered multiple times on each repaint. Even worse, frame()->isContentEditable() will traverse through the entire branch and most of time, does nothing. Right, it's quite horrible. Would you mind removing Frame::isContentEditable and adding a FIXME to EditorClient::clientIsEditable?

(In reply to comment #15) > (In reply to comment #14) > > I believe so. isContentEditable is called so frequently that I see it's triggered multiple times on each repaint. Even worse, frame()->isContentEditable() will traverse through the entire branch and most of time, does nothing. > > Right, it's quite horrible. Would you mind removing Frame::isContentEditable and adding a FIXME to EditorClient::clientIsEditable? I will be happy to work on the follow-up patches. How's this plan sound: 1. land this patch that fixes the designmode. 2. cleanup Frame::isContentEditable; 3. Work on 52058 (renaming Node::isContentEditible). 4. improve/fix clientIsEditable and other related issues.

Comment on attachment 80675 [details] fix patch 2 (In reply to comment #16) > (In reply to comment #15) > > Right, it's quite horrible. Would you mind removing Frame::isContentEditable and adding a FIXME to EditorClient::clientIsEditable? > > I will be happy to work on the follow-up patches. How's this plan sound: > 1. land this patch that fixes the designmode. > 2. cleanup Frame::isContentEditable; > 3. Work on 52058 (renaming Node::isContentEditible). > 4. improve/fix clientIsEditable and other related issues. Unfortunately, it can't be a followup. In this patch, you're eliminating the call to clientIsEditable and breaking the feature. So unless we completely unsupport this feature, our implementation becomes inconsistent. I suggest the following steps: 1. Land this patch after removing Frame::isContentEditable and fixing its call sites all in one patch 2. Remove EditorClient::clientIsEditable And 1 and 2 can't be broken down into smaller pieces. Alternatively, you can do: 1. Get rid of Frame::isContentEditable and remove EditorClient::clientIsEditable 2. Land this patch But this might be challenging since we seem to rely on Frame::isContentEditable for design-mode behavior.

Thanks so much for the suggestion. I will work on the next patch. > Unfortunately, it can't be a followup. In this patch, you're eliminating the call to clientIsEditable and breaking the feature. So unless we completely unsupport this feature, our implementation becomes inconsistent. I suggest the following steps: > > 1. Land this patch after removing Frame::isContentEditable and fixing its call sites all in one patch > 2. Remove EditorClient::clientIsEditable > > And 1 and 2 can't be broken down into smaller pieces. Alternatively, you can do: > > 1. Get rid of Frame::isContentEditable and remove EditorClient::clientIsEditable > 2. Land this patch > > But this might be challenging since we seem to rely on Frame::isContentEditable for design-mode behavior.

Created attachment 81715 [details] fix patch 3

Comment on attachment 81715 [details] fix patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=81715&action=review Could you also test a case where you set contenteditable programmatically? (i.e. div.contenteditable = false / true). r- for style issues. > LayoutTests/fast/dom/HTMLElement/iscontenteditable-designmodeon-allinherit.html:19 > +<div id="div1"> > + <div> > + <div id="div"></div> > + </div> > +</div> > +<div id="div2"> > + <div> > + <p id="p"></p> > + </div> Why do we need div1/div2 and empty divs? id's div1 and div2 can be eliminated since we're not referencing them anywhere. Please simplify the markup so that it doesn't clutter the test needlessly. > LayoutTests/fast/dom/HTMLElement/iscontenteditable-designmodeon-ancestor.html:11 > +<div id="div1" contentEditable="false"> div1 is not a descriptive name. How about parent_of_div? > LayoutTests/fast/dom/HTMLElement/iscontenteditable-designmodeon-ancestor.html:16 > +<div id="div2" contentEditable="true"> Ditto. parent_of_p?

Comment on attachment 81715 [details] fix patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=81715&action=review > Source/WebCore/page/Frame.cpp:-557 > - if (m_editor.clientIsEditable()) > - return true; Could you also file a follow up bug to remove contentIsEditable from EditorClient?

(In reply to comment #21) > (From update of attachment 81715 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81715&action=review > > > Source/WebCore/page/Frame.cpp:-557 > > - if (m_editor.clientIsEditable()) > > - return true; > > Could you also file a follow up bug to remove contentIsEditable from EditorClient? Nvm. Filed https://bugs.webkit.org/show_bug.cgi?id=54050 myself.

> > LayoutTests/fast/dom/HTMLElement/iscontenteditable-designmodeon-allinherit.html:19 > > +<div id="div1"> > > + <div> > > + <div id="div"></div> > > + </div> > > +</div> > > +<div id="div2"> > > + <div> > > + <p id="p"></p> > > + </div> > > Why do we need div1/div2 and empty divs? id's div1 and div2 can be eliminated since we're not referencing them anywhere. Please simplify the markup so that it doesn't clutter the test needlessly. Div1/div2 are used to call hide on them at the end of the test. Empty divs are used for making the tree a bit deeper. But I can remove the empty divs.

Comment on attachment 81715 [details] fix patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=81715&action=review > Source/WebCore/dom/Document.cpp:4020 > m_designMode = value; > + if (renderer()) { > + renderer()->style()->setUserModify((value == on) ? READ_WRITE : READ_ONLY); > + recalcStyle(Inherit); > + } One more concern. What if we created an iframe and set designmode true (when renderer still doesn't exist for the document), and then inserted the iframe into the document? Will designmode be still on?

> > Source/WebCore/dom/Document.cpp:4020 > > m_designMode = value; > > + if (renderer()) { > > + renderer()->style()->setUserModify((value == on) ? READ_WRITE : READ_ONLY); > > + recalcStyle(Inherit); > > + } > > One more concern. What if we created an iframe and set designmode true (when renderer still doesn't exist for the document), and then inserted the iframe into the document? Will designmode be still on? Designmode should be on in this case. However, the usermodify style may not sync up. I can work on this case further in a followup patch. In another way, if we remove the -webkit-user-modify support, this should not be a problem any more.

Created attachment 81810 [details] fix patch 4

(In reply to comment #25) > > One more concern. What if we created an iframe and set designmode true (when renderer still doesn't exist for the document), and then inserted the iframe into the document? Will designmode be still on? > > Designmode should be on in this case. However, the usermodify style may not sync up. I can work on this case further in a followup patch. In another way, if we remove the -webkit-user-modify support, this should not be a problem any more. What I'm suspecting is that this patch may introduce a regression. I'm not speaking of the value returned by isContentEditable. I'm saying that WebKit might mistakenly think that the document is not editable or more precisely that WebKit may not store the necessarily information to infer that the document is editable if we landed your patch because we no longer call EditorClient::contentIsEditable. To fix this problem, you'll probably need to call setUserModify when the document's renderer is attached.

> To fix this problem, you'll probably need to call setUserModify when the document's renderer is attached. Would it be ok to address this in a follow-up patch? The code change may be simple but I have to come up with a set of test cases.

(In reply to comment #28) > > To fix this problem, you'll probably need to call setUserModify when the document's renderer is attached. > > Would it be ok to address this in a follow-up patch? The code change may be simple but I have to come up with a set of test cases. The answer to that question depends on whether or not this patch will cause a regression. My suspicion is that it does in which case you can't do it in a follow-up patch.

(In reply to comment #29) > The answer to that question depends on whether or not this patch will cause a regression. My suspicion is that it does in which case you can't do it in a follow-up patch. Ok, I think my concern was a false alarm. > > > Source/WebCore/dom/Document.cpp:4020 > > > m_designMode = value; > > > + if (renderer()) { > > > + renderer()->style()->setUserModify((value == on) ? READ_WRITE : READ_ONLY); > > > + recalcStyle(Inherit); > > > + } > > > > One more concern. What if we created an iframe and set designmode true (when renderer still doesn't exist for the document), and then inserted the iframe into the document? Will designmode be still on? > > Designmode should be on in this case. However, the usermodify style may not sync up. I can work on this case further in a followup patch. In another way, if we remove the -webkit-user-modify support, this should not be a problem any more. We should add ASSERT(renderer()) right above the if statement. It's still nice to have an early exit in the case our assumption that the document's renderer always exists was false.

Created attachment 82050 [details] fix patch 5 new patch based on rniwa's comments.

Comment on attachment 82050 [details] fix patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=82050&action=review Sorry I didn’t get a chance to look at this before. > Source/WebCore/dom/Document.cpp:4033 > + ASSERT(renderer()); > + if (renderer()) { > + renderer()->style()->setUserModify((value == on) ? READ_WRITE : READ_ONLY); > + recalcStyle(Inherit); > + } This is not the right way to make style depend on something. This logic should go into CSSStyleSelector::styleForDocument instead of here. Reaching right in and tweaking the style isn’t the way to do it. It’s also wrong to call recalcStyle synchronously. Instead we should be calling scheduleForcedStyleRecalc here, and calling it only if the new value is different from m_designMode.

> > Source/WebCore/dom/Document.cpp:4033 > > + ASSERT(renderer()); > > + if (renderer()) { > > + renderer()->style()->setUserModify((value == on) ? READ_WRITE : READ_ONLY); > > + recalcStyle(Inherit); > > + } > > This is not the right way to make style depend on something. This logic should go into CSSStyleSelector::styleForDocument instead of here. Reaching right in and tweaking the style isn’t the way to do it. > > It’s also wrong to call recalcStyle synchronously. Instead we should be calling scheduleForcedStyleRecalc here, and calling it only if the new value is different from m_designMode. Thanks for showing the right way to do this, Darin. I have tried and found it results a similar issue with other setContentEditable test cases that the render style wasn't be updated before the execution of the next js statement. This must be due to the async call to recalcStyle. But if this is the current behavior, I may have to submit it as it is?

(In reply to comment #33) > > This is not the right way to make style depend on something. This logic should go into CSSStyleSelector::styleForDocument instead of here. Reaching right in and tweaking the style isn’t the way to do it. > > > > It’s also wrong to call recalcStyle synchronously. Instead we should be calling scheduleForcedStyleRecalc here, and calling it only if the new value is different from m_designMode. > > Thanks for showing the right way to do this, Darin. I have tried and found it results a similar issue with other setContentEditable test cases that the render style wasn't be updated before the execution of the next js statement. This must be due to the async call to recalcStyle. But if this is the current behavior, I may have to submit it as it is? I think that means you need to fix the other bug first.

> I think that means you need to fix the other bug first. That means we have to remove the support of -webkit-user-modify. Do we really want to do that?

(In reply to comment #35) > > I think that means you need to fix the other bug first. > > That means we have to remove the support of -webkit-user-modify. Do we really want to do that? I think we should just have two versions of isContentEditable just as Darin suggested.

(In reply to comment #36) > (In reply to comment #35) > > > I think that means you need to fix the other bug first. > > > > That means we have to remove the support of -webkit-user-modify. Do we really want to do that? > > I think we should just have two versions of isContentEditable just as Darin suggested. Right, looks like we should do that first. It would be massive change so I planned to do later.

I am trying to make the test cases in "fix patch 5" work. The test cases require that contentEditable attribute should overwrite the documentDesignMode setting. But when it's not there, the editability depends on designMode. The current implementation in Node::rendererIsEditable seems wrong. It always checks inDesignMode first, which will ignore local contentEditable. But even if we check local renderstyle first, we still don't know if userModify() is set to be false by setContentEditable or by default. To make the render-style approach work, I think we have to utilize the Inherit value. In HTMLElement::setContentEditable function, the code indeed tries to call addCSSProperty(attr, CSSPropertyWebkitUserModify, CSSValueInherit). However, the code is broken because the integer value of CSSValueInherit does not map to EUserModify. See the code below: template<> inline CSSPrimitiveValue::operator EUserModify() const { return static_cast<EUserModify>(m_value.ident - CSSValueReadOnly); } I think we should fix this. Do I make myself clear? Any ideas on an alternative approach? Thanks!

(In reply to comment #38) > The current implementation in Node::rendererIsEditable seems wrong. It always checks inDesignMode first, which will ignore local contentEditable. But even if we check local renderstyle first, we still don't know if userModify() is set to be false by setContentEditable or by default. Is this behavior spec-ed somewhere? It'll be nice to be able to cite the part of spec that mandates this behavior. > To make the render-style approach work, I think we have to utilize the Inherit value. In HTMLElement::setContentEditable function, the code indeed tries to call addCSSProperty(attr, CSSPropertyWebkitUserModify, CSSValueInherit). However, the code is broken because the integer value of CSSValueInherit does not map to EUserModify. See the code below: > template<> inline CSSPrimitiveValue::operator EUserModify() const > { > return static_cast<EUserModify>(m_value.ident - CSSValueReadOnly); > } > > I think we should fix this. Sounds good to me.

> Is this behavior spec-ed somewhere? It'll be nice to be able to cite the part of spec that mandates this behavior. I had some comments in the ChangeLog. The exact cite is too long and contains other stuff. Check this out: http://dev.w3.org/html5/spec/Overview.html#attr-contenteditable "Specifically, if an HTML element has a contenteditable attribute set to the true state, or it has its contenteditable attribute set to the inherit state and if its nearest ancestor HTML element with the contenteditable attribute set to a state other than the inherit state has its attribute set to the true state, or if it and its ancestors all have their contenteditable attribute set to the inherit state but the Document has designMode enabled, then the UA must treat the element as editable (as described below)." "Otherwise, either the HTML element has a contenteditable attribute set to the false state, or its contenteditable attribute is set to the inherit state and its nearest ancestor HTML element with the contenteditable attribute set to a state other than the inherit state has its attribute set to the false state, or all its ancestors have their contenteditable attribute set to the inherit state and the Document itself has designMode disabled; either way, the element is not editable." > > I think we should fix this. > > Sounds good to me. Good.

Thank you for the clarification, Chang. Now that I understand the correct behavior, we should proceed with your proposed design changes.

(In reply to comment #41) > Thank you for the clarification, Chang. Now that I understand the correct behavior, we should proceed with your proposed design changes. Thanks for the support!

Created attachment 88632 [details] fix patch: new patch after dependencies have been resolved.

Comment on attachment 88632 [details] fix patch: new patch after dependencies have been resolved. View in context: https://bugs.webkit.org/attachment.cgi?id=88632&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1229 > + if (style->userModify() == INHERIT) > + return primitiveValueCache->createValue(node->document()->inDesignMode() ? READ_WRITE : READ_ONLY); I don't think this is what you want. As Darin suggested you should set the value in CSSStyleSelector::styleForDocument and things should work just as expected. > Source/WebCore/css/CSSPrimitiveValueMappings.h:1824 > + case INHERIT: > + m_value.ident = CSSValueInherit; > + break; I don't think this is right. > Source/WebCore/css/CSSPrimitiveValueMappings.h:1831 > + if (m_value.ident == CSSValueInherit) > + return INHERIT; Ditto. > Source/WebCore/dom/Node.cpp:720 > - if (document()->inDesignMode() || (document()->frame() && document()->frame()->page() && document()->frame()->page()->isEditable())) > + if (document()->frame() && document()->frame()->page() && document()->frame()->page()->isEditable()) Removing this call is good because if you modify CSSStyleSelector::styleForDocument, then the value will automatically be inherited. > Source/WebCore/dom/Node.cpp:737 > + case INHERIT: > + return document()->inDesignMode(); Then you don't need this work-around.

Thanks for the review. I will work on the new approach soon. (In reply to comment #44) > (From update of attachment 88632 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88632&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1229 > > + if (style->userModify() == INHERIT) > > + return primitiveValueCache->createValue(node->document()->inDesignMode() ? READ_WRITE : READ_ONLY); > > I don't think this is what you want. As Darin suggested you should set the value in CSSStyleSelector::styleForDocument and things should work just as expected. > > > Source/WebCore/css/CSSPrimitiveValueMappings.h:1824 > > + case INHERIT: > > + m_value.ident = CSSValueInherit; > > + break; > > I don't think this is right. > > > Source/WebCore/css/CSSPrimitiveValueMappings.h:1831 > > + if (m_value.ident == CSSValueInherit) > > + return INHERIT; > > Ditto. > > > Source/WebCore/dom/Node.cpp:720 > > - if (document()->inDesignMode() || (document()->frame() && document()->frame()->page() && document()->frame()->page()->isEditable())) > > + if (document()->frame() && document()->frame()->page() && document()->frame()->page()->isEditable()) > > Removing this call is good because if you modify CSSStyleSelector::styleForDocument, then the value will automatically be inherited. > > > Source/WebCore/dom/Node.cpp:737 > > + case INHERIT: > > + return document()->inDesignMode(); > > Then you don't need this work-around.

Created attachment 89262 [details] new patch #2

Comment on attachment 89262 [details] new patch #2 View in context: https://bugs.webkit.org/attachment.cgi?id=89262&action=review The patch looks better than the last one but I don't know enough about layout/style code to know if your change is correct. > Source/WebCore/css/CSSStyleSelector.cpp:1268 > + if (document->inDesignMode() && documentStyle->userModify() != READ_WRITE) Do we really need to check documentStyle->userModify() != READ_WRITE? That seems like a non-standard thing to do here. > Source/WebCore/dom/Document.cpp:4068 > + scheduleForcedStyleRecalc(); I'm not sure if this is the correct function to call. Also, don't we need to update styles of subframes?

> > Source/WebCore/css/CSSStyleSelector.cpp:1268 > > + if (document->inDesignMode() && documentStyle->userModify() != READ_WRITE) > > Do we really need to check documentStyle->userModify() != READ_WRITE? That seems like a non-standard thing to do here. Just want to check if we really have to change the value. > > Source/WebCore/dom/Document.cpp:4068 > > + scheduleForcedStyleRecalc(); > > I'm not sure if this is the correct function to call. Also, don't we need to update styles of subframes? From the code, it looks that only the "forced" one will trigger a styleForDocument recalculation. For subframes, I think document.designmode is per document, so it's per frame? Anyway, let's see if any other reviewers want to comment. Thanks!

(In reply to comment #48) > > > Source/WebCore/css/CSSStyleSelector.cpp:1268 > > > + if (document->inDesignMode() && documentStyle->userModify() != READ_WRITE) > > > > Do we really need to check documentStyle->userModify() != READ_WRITE? That seems like a non-standard thing to do here. > > Just want to check if we really have to change the value. I don't think we need to. If it's the same value, then there's no harm in changing it to the same value. > > > Source/WebCore/dom/Document.cpp:4068 > > > + scheduleForcedStyleRecalc(); > > > > I'm not sure if this is the correct function to call. Also, don't we need to update styles of subframes? > > From the code, it looks that only the "forced" one will trigger a styleForDocument recalculation. For subframes, I think document.designmode is per document, so it's per frame? No. See http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp?rev=83349#L4075

Per discussion with smfr on IRC, you should probably use SyntheticStyleChange and call style recalc on all subframes.

> Per discussion with smfr on IRC, you should probably use SyntheticStyleChange and call style recalc on all subframes. Great, will try.

(In reply to comment #50) > Per discussion with smfr on IRC, you should probably use SyntheticStyleChange and call style recalc on all subframes. I replaced scheduleForcedStyleRecalc with setNeedsStyleRecalc(SyntheticStyleChange) in Document::setDesignMode, but it didn't work for me. From the code inside function Document::recalcStyle(), calling it with Force is the only way to trigger CSSStyleSelector::styleForDocument().

Created attachment 90374 [details] fix patch: WIP This is a work-in-progress patch. It crashes editing/style/iframe-onload-crash-mac.html because of an infinite loop.

Created attachment 90448 [details] fixed the crashing tests

(In reply to comment #54) > Created an attachment (id=90448) [details] > fixed the crashing tests ojan and I investigated this test and concluded that the test is inherently wrong and we need to fix the test to avoid infinite recursion. I have set a limit of 16 recursions in the test and fixed one place in ApplyStyleCommand to avoid an assertion failure (DOM mutation events can change the editability and the code wasn't accounting this). I'm the original author of the test and confirmed that the original bug reproduces with the modified test.

(In reply to comment #55) > (In reply to comment #54) > > Created an attachment (id=90448) [details] [details] > > fixed the crashing tests > > ojan and I investigated this test and concluded that the test is inherently wrong and we need to fix the test to avoid infinite recursion. I have set a limit of 16 recursions in the test and fixed one place in ApplyStyleCommand to avoid an assertion failure (DOM mutation events can change the editability and the code wasn't accounting this). I'm the original author of the test and confirmed that the original bug reproduces with the modified test. Thanks, Ryosuke, for fixing the test and the code. I suggest we take out the updated test cases and ApplyStyleCommand fix in a separate patch and probably put in for a new bug. The code in Document.cpp in your patch has a redundant call to stylerecalc and my new tests are missing.

(In reply to comment #56) > Thanks, Ryosuke, for fixing the test and the code. I suggest we take out the updated test cases and ApplyStyleCommand fix in a separate patch and probably put in for a new bug. The code in Document.cpp in your patch has a redundant call to stylerecalc and my new tests are missing. On my second thought, you need to include my change in your patch because the problem I'm fixing won't reproduce without your change. It seems odd to fix a problem that doesn't exist in a separate patch.

That makes sense, too. I will take care of it, thanks. > On my second thought, you need to include my change in your patch because the problem I'm fixing won't reproduce without your change. It seems odd to fix a problem that doesn't exist in a separate patch.

Created attachment 90704 [details] fix patch: merged Ryosuke's code.

Comment on attachment 90704 [details] fix patch: merged Ryosuke's code. View in context: https://bugs.webkit.org/attachment.cgi?id=90704&action=review > Source/WebCore/dom/Document.cpp:4037 > + for (Frame* frame = m_frame; frame && frame->document(); frame = frame->tree()->traverseNext(m_frame)) So this will work on all subframes? Will this end up recalcing style on parent frames as well? > LayoutTests/editing/style/iframe-onload-crash-mac.html:10 > +if (document.counter) > + document.counter++; > +else > + document.counter = 1; I don't understand what the purpose of these counters are. > LayoutTests/editing/style/iframe-onload-crash-win.html:10 > +if (document.counter) > + document.counter++; > +else > + document.counter = 1; Huh?

> > + for (Frame* frame = m_frame; frame && frame->document(); frame = frame->tree()->traverseNext(m_frame)) > > So this will work on all subframes? Will this end up recalcing style on parent frames as well? Yes, We have to do this because DesignMode inherits from parent frames. No, traverseNext(m_frame) makes sure traversing happens within m_frame. > > LayoutTests/editing/style/iframe-onload-crash-mac.html:10 > > +if (document.counter) > > + document.counter++; > > +else > > + document.counter = 1; > > I don't understand what the purpose of these counters are. The test cases will trigger an infinite loop because the execCommand() in the test will reload the frame which calls these execCommand() again and again.

Comment on attachment 90704 [details] fix patch: merged Ryosuke's code. View in context: https://bugs.webkit.org/attachment.cgi?id=90704&action=review >> LayoutTests/editing/style/iframe-onload-crash-mac.html:10 >> + document.counter = 1; > > I don't understand what the purpose of these counters are. Without this counter, the test falls into an infinite loop because this function is called on load event of an iframe and it moves iframe (removes & appends back) in editing commands. We didn't have this problem before this patch due to the bug this patch is fixing.

Comment on attachment 90704 [details] fix patch: merged Ryosuke's code. View in context: https://bugs.webkit.org/attachment.cgi?id=90704&action=review > LayoutTests/fast/dom/HTMLElement/iscontenteditable-designmodeon-expected.txt:10 > + > + > + > + > + > + What's with the gigantic whitespace blob? > LayoutTests/fast/dom/HTMLElement/iscontenteditable-designmodeon-expected.txt:36 > + > + > + > + Here too.

Comment on attachment 90704 [details] fix patch: merged Ryosuke's code. This seems fine.

(In reply to comment #63) > (From update of attachment 90704 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90704&action=review > > > LayoutTests/fast/dom/HTMLElement/iscontenteditable-designmodeon-expected.txt:10 > > + > > + > > + > > + > > + > > + > > What's with the gigantic whitespace blob? > > > LayoutTests/fast/dom/HTMLElement/iscontenteditable-designmodeon-expected.txt:36 > > + > > + > > + > > + > > Here too. I didn't see these comments before I did cq+. The white spaces were created because of the <div>s and <p>s.

The commit-queue encountered the following flaky tests while processing attachment 90704 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: dumi@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 90704 [details] fix patch: merged Ryosuke's code. Clearing flags on attachment: 90704 Committed r85267: <http://trac.webkit.org/changeset/85267>

All reviewed patches have been landed. Closing bug.

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