Bug 53031 – isContentEditable is not working properly with document.designMode

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chang Shu]

isContentEditable should not change if its or its ancestor's contentEditable is set, no matter whether document.designMode is enabled or not.

[Chang Shu]

Created attachment 79950 [details]
fix patch

[Ryosuke Niwa]

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.

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 79950 [details]
fix patch

Restoring the review-.

[Chang Shu]

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.

[Chang Shu]

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

I mean, non-trivial. :)

[Darin Adler]

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

[Chang Shu]

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?

[Darin Adler]

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

[Chang Shu]

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

[Chang Shu]

Created attachment 80675 [details]
fix patch 2

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Chang Shu]

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

[Ryosuke Niwa]

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

[Chang Shu]

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

[Ryosuke Niwa]

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.

[Chang Shu]

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.

[Chang Shu]

Created attachment 81715 [details]
fix patch 3

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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

[Chang Shu]

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

[Ryosuke Niwa]

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?

[Chang Shu]

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

[Chang Shu]

Created attachment 81810 [details]
fix patch 4

[Ryosuke Niwa]

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

[Chang Shu]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Chang Shu]

Created attachment 82050 [details]
fix patch 5

new patch based on rniwa's comments.

[Darin Adler]

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.

[Chang Shu]

> > 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?

[Ryosuke Niwa]

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

[Chang Shu]

> 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?

[Ryosuke Niwa]

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

[Chang Shu]

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

[Chang Shu]

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!

[Ryosuke Niwa]

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

[Chang Shu]

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

[Ryosuke Niwa]

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

[Chang Shu]

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

[Chang Shu]

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

[Ryosuke Niwa]

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.

[Chang Shu]

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.

[Chang Shu]

Created attachment 89262 [details]
new patch #2

[Ryosuke Niwa]

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?

[Chang Shu]

> > 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!

[Ryosuke Niwa]

(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

[Ryosuke Niwa]

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

[Chang Shu]

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

Great, will try.

[Chang Shu]

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

[Chang Shu]

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.

[Ryosuke Niwa]

Created attachment 90448 [details]
fixed the crashing tests

[Ryosuke Niwa]

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

[Chang Shu]

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

[Ryosuke Niwa]

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

[Chang Shu]

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.

[Chang Shu]

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

[Eric Seidel (no email)]

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?

[Chang Shu]

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

[Ryosuke Niwa]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

This seems fine.

[Chang Shu]

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

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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

Clearing flags on attachment: 90704

Committed r85267: <http://trac.webkit.org/changeset/85267>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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