Closed Bug 694880 Opened 11 years ago Closed 11 years ago

The editable state is not updated correctly when designMode is turned off


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)




(2 files)

Attached file Testcase
See the attached test case.

The bug is caused because of the incorrect assumption made by NotifyEditableStateChange.  This function assumes that when designMode is turned on, we propagate the NODE_IS_EDITABLE flag throughout the document, so it tries to optimize not calling UpdateState if the value of the flag has not changed.

When the designMode is turned off, none of the nodes will have the NODE_IS_EDITABLE_FLAG, so the check will always fail, and UpdateState will never be called, leaving all of the nodes in a document with an incorrect state.
Also, if there is no contentEditable element in the document (like in the case of the testcase), NotifyEditingStateChanged is not called at all...
Attached patch Patch (v1)Splinter Review
Assignee: nobody → ehsan
Attachment #567364 - Flags: review?(bzbarsky)
Blocks: 688438
Try run for f0b477074232 is complete.
Detailed breakdown of the results available here:
Results (out of 193 total builds):
    success: 173
    warnings: 10
    failure: 10
Builds available at
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 567364 [details] [diff] [review]
Patch (v1)

Probably fine; can you get peterv to give this a once-over, though?
Attachment #567364 - Flags: review?(peterv)
Attachment #567364 - Flags: review?(bzbarsky)
Attachment #567364 - Flags: review+
Comment on attachment 567364 [details] [diff] [review]
Patch (v1)

Isn't the problem that NotifyEditableStateChange should call |child->IsEditable()| instead of |child->HasFlag(NODE_IS_EDITABLE)|? IIRC we tried to avoid walking the whole document when designMode was toggled, and so IntrinsicState() checks both the node and its document before returning NS_EVENT_STATE_MOZ_READONLY. If we want to change that (walk the whole document when designMode is toggled) we should go the whole way and make IntrinsicState() just check the node. There might be more changes needed though.
We store the state in the node directly now, so if the state of all the nodes changed we need to walk the whole document.
Yes, and this is where the bug is stemming from.
We're still only setting the NODE_IS_EDITABLE flag on non-document nodes for contentEditable, designMode doesn't affect that. So why do we need to drop the |child->HasFlag(NODE_IS_EDITABLE) != aEditable| check instead of making it work for designMode?
All the testcases in the patch seem to pass with just this change:

-    if (child->HasFlag(NODE_IS_EDITABLE) != aEditable &&
-        child->IsElement()) {
+    if (child->IsEditable() != aEditable && child->IsElement()) {
Attachment #567364 - Flags: review?(peterv) → review+
(In reply to Peter Van der Beken [:peterv] from comment #10)
> All the testcases in the patch seem to pass with just this change:
> -    if (child->HasFlag(NODE_IS_EDITABLE) != aEditable &&
> -        child->IsElement()) {
> +    if (child->IsEditable() != aEditable && child->IsElement()) {

That would break the test case for bug 672709.
Flags: in-testsuite+
Target Milestone: --- → mozilla10
Closed: 11 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.