Last Comment Bug 694880 - The editable state is not updated correctly when designMode is turned off
: The editable state is not updated correctly when designMode is turned off
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: :Ehsan Akhgari
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 688438
  Show dependency treegraph
 
Reported: 2011-10-16 15:24 PDT by :Ehsan Akhgari
Modified: 2011-10-21 02:15 PDT (History)
6 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (233 bytes, text/html)
2011-10-16 15:24 PDT, :Ehsan Akhgari
no flags Details
Patch (v1) (4.86 KB, patch)
2011-10-16 15:46 PDT, :Ehsan Akhgari
bzbarsky: review+
peterv: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-10-16 15:24:12 PDT
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.
Comment 1 :Ehsan Akhgari 2011-10-16 15:40:40 PDT
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 :Ehsan Akhgari 2011-10-16 15:46:43 PDT
Created attachment 567364 [details] [diff] [review]
Patch (v1)
Comment 3 Mozilla RelEng Bot 2011-10-16 23:30:49 PDT
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
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-10-17 10:31:39 PDT
Comment on attachment 567364 [details] [diff] [review]
Patch (v1)

Probably fine; can you get peterv to give this a once-over, though?
Comment 5 :Ehsan Akhgari 2011-10-17 13:03:11 PDT
Comment on attachment 567364 [details] [diff] [review]
Patch (v1)

Sure.
Comment 6 Peter Van der Beken [:peterv] 2011-10-18 06:18:24 PDT
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 Boris Zbarsky [:bz] (still a bit busy) 2011-10-18 06:44:09 PDT
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 :Ehsan Akhgari 2011-10-18 10:33:29 PDT
Yes, and this is where the bug is stemming from.
Comment 9 Peter Van der Beken [:peterv] 2011-10-18 12:10:50 PDT
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?
Comment 10 Peter Van der Beken [:peterv] 2011-10-18 12:15:14 PDT
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()) {
Comment 11 :Ehsan Akhgari 2011-10-20 09:25:26 PDT
(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 13 Marco Bonardo [::mak] 2011-10-21 02:15:37 PDT
https://hg.mozilla.org/mozilla-central/rev/7324c75b47ca

Note You need to log in before you can comment on or make changes to this bug.