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

RESOLVED FIXED in mozilla10



7 years ago
7 years ago


(Reporter: Ehsan, Assigned: Ehsan)


Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(2 attachments)



7 years ago
Created attachment 567360 [details]

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.

Comment 1

7 years ago
Also, if there is no contentEditable element in the document (like in the case of the testcase), NotifyEditingStateChanged is not called at all...

Comment 2

7 years ago
Created attachment 567364 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Attachment #567364 - Flags: review?(bzbarsky)


7 years ago
Blocks: 688438

Comment 3

7 years ago
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 5

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

Comment 8

7 years ago
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+

Comment 11

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

Comment 12

7 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla10
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.