290261 - Midas editor undo breaks after HTML inserted through .innerHTML

Status: NEW

(Core :: DOM: Editor, defect, P5)

Description

[Derek Davenport]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050413 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050413 Firefox/1.0+

This may be related to bug <a
href="https://bugzilla.mozilla.org/show_bug.cgi?id=116045">116045</a>.
After you use .innerHTML to insert HTML into a midas editor trying to use Undo
gives the following error:
Error: uncaught exception: [Exception... "Node was not found"  code: "8"
nsresult: "0x80530008 (NS_ERROR_DOM_NOT_FOUND_ERR)"  location:
"http://www.mozilla.org/editor/midasdemo/ Line: 199"]
Note that editor.contentDocument.queryCommandEnabled("Undo") returns true.
After this trying to undo or redo gives this error:
Error: uncaught exception: [Exception... "Index or size is negative or greater
than the allowed amount"  code: "1" nsresult: "0x80530001
(NS_ERROR_DOM_INDEX_SIZE_ERR)"  location:
"http://www.mozilla.org/editor/midasdemo/ Line: 199"]

Reproducible: Always

Steps to Reproduce:
1. Visit midas demo
2. Type some text into the editor
3. Click "View HTML Source" checkbox twice (uses .innerHTML)
4. Click the undo button for first error
5. Click undo/redo for more errors

Actual Results:  
Exception thrown when you click undo
Subsequent exceptions on undo/redo commands

Expected Results:  
editor.contentDocument.queryCommandEnabled("Undo") should return false
undo command should do nothing
redo command should do nothing

Comment 1

[Hiro]

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

Comment 2

[Hiro]

Confirm.

It is an operation of Undo and the following errors are in JavaScript Console. 

Error: uncaught exception: [Exception... "Index or size is negative or greater
than the allowed amount"  code: "1" nsresult: "0x80530001
(NS_ERROR_DOM_INDEX_SIZE_ERR)"  location:
"http://www.mozilla.org/editor/midasdemo/ Line: 199"]

Windows XP SP1
Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.8b2) Gecko/20050413
Firefox/1.0+

Comment 3

[Boris Zbarsky [:bzbarsky]]

The problem is that undo undoes an operation on DOM nodes.  Changing innerHTML
wipes out the DOM rooted at the node whose innerHTML is being changed competely.

What we should probably do is simply clear the undo history when innerHTML is
used (or when any other DOM mutation not under editor control is performed, for
that matter).

Comment 4

[Kathleen :Brade]

I agree with bz (comment 3).

IIRC Composer uses .innerHTML for changing modes (to and from html source) and
it explicitly clears the undo stack (iirc, it makes the "set new html source"
one transaction so you can undo all of the html source edits).

Does the DOM have an observer or another way to know when major changes happen
to it?

Comment 5

[Boris Zbarsky [:bzbarsky]]

You can set mutation event listeners (and make sure not to trigger when you're
making changes yourself....).

Comment 6

[Hixie (not reading bugmail)]

In IE, during a drag-and-drop operation (from event "dragstart" to event
"dragend") almost _any_ DOM change is considered to be undoable. This includes
script setting innerHTML. HTML5 will be requiring this too. So it looks like
"we" just have to find a way to have undo work across innerHTML changes. :-)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ian, that's not the poing.  Conceivably we _could_ undo setInnerHTML.  The
problem is that people are using the editor transaction api for undo/redo while
at the same time performing DOM mutations that the editor isn't notified about.
 So they do action A in editor, then rearrange the DOM, then request that action
A be undone.  The request makes no sense, given the rearrangement of the DOM.

As for HTML5, it can require whatever it wants, as can other specs.  We'll
implement as time permits and as it's considered useful (on a par with other specs).

Comment 8

[Hixie (not reading bugmail)]

(In reply to comment #7)
> Ian, that's not the point.  Conceivably we _could_ undo setInnerHTML.  The
> problem is that people are using the editor transaction api for undo/redo 
> while at the same time performing DOM mutations that the editor isn't notified
> about.

Ah, ok. Well, in IE DOM manipulation that is not done as part of an editor
operation or part of a drag-and-drop sequence does reset the undo history as far
as I can tell. However, during drag-and-drop _any_ change, including changes
caused by setTimeouts or event handlers, will be put into one atomic operation
that can be undone in one go after the drop.


> As for HTML5, it can require whatever it wants, as can other specs.  We'll
> implement as time permits and as it's considered useful (on a par with other 
> specs).

There's no point HTML5 saying anything that Mozilla isn't going to implement, so
please don't say it can "require whatever it wants". It can't. If you don't
consider something in the HTML5 spec to be useful, or if you don't think it
would be implementable in reasonable time, then please say so, so that we can
change the spec.

Comment 9

[Boris Zbarsky [:bzbarsky]]

What do you consider reasonable time?

Comment 10

[Hixie (not reading bugmail)]

s/implementable in reasonable time/implemented before you stop working on gecko/.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well, the short of it is that editor back end is currently not being actively
developed in the mozilla.org tree...  Unless that changes, I'd say that anything
that specifies editor behavior is not that likely to be implemented in the short
(Gecko 2.0) term.

Comment 12

[Hixie (not reading bugmail)]

That's fair enough.

Comment 13

[Derek Davenport]

appendChild, insertBefore, and the like can also cause this error. A lot of
editors need to add stuff that execCommand doesn't support. Such as in the midas
demo try typing something, selecting it, adding a table, and then hitting undo.
All DOM changes should be part of the undo history.

Comment 14

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, >=5 years untouched DOM/Storage bugs' priority.

If you have reason to believe this is wrong (especially for the severity), please write a comment and ni :jstutte.