Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla10

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla10
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 567360 [details]
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...
Created attachment 567364 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #567364 - Flags: review?(bzbarsky)
Blocks: 688438

Comment 3

6 years ago
Try run for f0b477074232 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f0b477074232
Results (out of 193 total builds):
    success: 173
    warnings: 10
    failure: 10
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-f0b477074232
OS: Mac OS X → All
Hardware: x86 → All

Comment 4

6 years ago
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)

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

Comment 7

6 years ago
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7324c75b47ca
Flags: in-testsuite+
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/7324c75b47ca
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.