1015433 - Undo/redo corrupts editing text (contenteditable or designmode)

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Cacycle]

Attachment: testcase-undo-redo.html

In (contenteditable div or iframe in designmode), undo/redo deletes text. Please see the attached testcase. Example html code:

1. <i><b>THIS TEXT DISAPPEARS</b> when copying [THIS] and pasting it here: []</i> and then pushing Ctrl-Z (or Undo) followed by Ctrl-Y (or Redo).
2. <span><span>DISAPPEARS</span>copypaste</span>

For Ctrl-X/Ctrl-Y, no error or warning is shown, for Undo/Redo buttons (execCommand), the error is: For NS_ERROR_ILLEGAL_VALUE. The problem is unrelated to the actual text/chars, the html inline tags used (span, b, or i), or contenteditable div vs. iframe in designmode.

This is a ***major*** problem as it corrupts existing content (e.g. on Wikipedia and other wikis) and this corruption might go unnoticed. Also, it breaks the undo/redo functionality in editors (such as the Wikipedia editor wikEd).

Comment 1

[Cacycle]

It's almost certainly a regression.

Comment 2

[Cacycle]

Happens in FF 29.0.1 with all add-ons disabled.

Comment 3

[(no longer active)]

(In reply to Cacycle from comment #1)
> It's almost certainly a regression.

Do you have a regression range?

Comment 4

[Alice0775 White]

Regresstion window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a38692ea87ae
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130109 Firefox/21.0 ID:20130109144609
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f21c83377345
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130109 Firefox/21.0 ID:20130109150608
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a38692ea87ae&tochange=f21c83377345

Regressed by:
c0f31a982bd4	David Zbarsky — Bug 828169 - Don't use nsIDOMNode in editor transactions r=ehsan

Comment 5

[David Zbarsky (:dzbarsky)]

That patch added some |NS_ENSURE_SUCCESS(result, result);| calls that weren't there previously.  I'll check if one of those is responsible.

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: Attempt

Here's the patch I tried.  Unfortunately it doesn't work.  The rest of the original patch shouldn't change any behavior as far as I can tell.

Comment 7

[Alice0775 White]

In localbuild,
Last Good: 9297bf41ab4d
First Bad: c0f31a982bd4

This bug is definitely caused by 'c0f31a982bd4 David Zbarsky — Bug 828169 - Don't use nsIDOMNode in editor transactions r=ehsan'

Comment 8

[(no longer active)]

David, can you take this please?

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

This is something that would be great to fix, of course, but it's been around since FF21 without any significant cry out from users in our usual feedback channels so there's no reason to track this but instead we can consider the uplift request when a patch is ready and depending on where we are in the cycle.  We're getting towards the end of FF30 Beta and this isn't something worth cramming in without time to get proper testing for other possible regressions.

Comment 10

[Cacycle]

Actually, this is a major problem as it corrupts existing content on Wikipedia and other wikis and this corruption might go unnoticed. Also, it breaks not only the undo/redo functionality in every single existing rich-text-editor, it also breaks something such basic as pasting text in the Wikipedia editor wikEd.

<!-- unrelated to fixing this specific bug --><rant steam="on"> The reason you did not here the outcry is probably that these bugs are attributed to the external applications (i.e. rich-text-editors). And it takes so much time and effort to track these Mozilla bugs down, to present good bug reports, and to develop workarounds. Often only to watch many of these bugs getting ignored, some for years (you can find my own prioritized list here: https://en.wikipedia.org/wiki/User_talk:Cacycle/wikEd). I understand that it is probably more sexy for browser developers to add new features and to prioritize away boring bug fixing. But it is so frustrating and time consuming for developers who have to work with messy browser editors that often feel like unmaintained construction sites... </rant><!-- steam off -->

Comment 11

[Alice0775 White]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #8)
> David, can you take this please?

setting flag need info.

Comment 12

[(no longer active)]

(In reply to Cacycle from comment #10)
> Actually, this is a major problem as it corrupts existing content on
> Wikipedia and other wikis and this corruption might go unnoticed. Also, it
> breaks not only the undo/redo functionality in every single existing
> rich-text-editor, it also breaks something such basic as pasting text in the
> Wikipedia editor wikEd.

Yes, understood.  I agree with the assessment in comment 9 about beta mostly because of timeline issues, but we should absolutely fix this in Firefox 31 if 30 won't be an option.

> <!-- unrelated to fixing this specific bug --><rant steam="on"> The reason
> you did not here the outcry is probably that these bugs are attributed to
> the external applications (i.e. rich-text-editors).

Yes, you're absolutely right.  There is also the additional problem of the HTML editing facilities in browser engines being so buggy that my guess is that some people just ignore those bugs on principle.  IOW, I don't think we should read anything into not hearing a public outcry about an editor regression.

> And it takes so much
> time and effort to track these Mozilla bugs down, to present good bug
> reports, and to develop workarounds.

Yes indeed.  And I do appreciate this bug report, it has a great test case which really helped us track this regression down.

> Often only to watch many of these bugs
> getting ignored, some for years (you can find my own prioritized list here:
> https://en.wikipedia.org/wiki/User_talk:Cacycle/wikEd). I understand that it
> is probably more sexy for browser developers to add new features and to
> prioritize away boring bug fixing. But it is so frustrating and time
> consuming for developers who have to work with messy browser editors that
> often feel like unmaintained construction sites... </rant><!-- steam off -->

Speaking about the Editor module in Gecko, it's unfortunately unowned in practice.  I'm the module owner but have not spent a significant amount of time working on this component for the past 2+ years, and we don't really have any active developer who's working on this code.  But I do care a lot about not breaking things that used to work...

Jet, Johnny, we definitely need to find someone to fix this.  Can you guys help, please?  Thanks!

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

OK we can track this for 31/32 because of the impact to all rich text editors (and that is not a desired impact at all) but this is something that would need to land in Aurora or *maybe* the first Beta of 31 if it will be in 31 so we have lots of bake time with the larger user population before shipping.

Comment 14

[David Zbarsky (:dzbarsky)]

I don't have time to investigate this at the moment, I think we should back out the regressing changeset.  I'll try to prepare a backout soon.

Comment 15

[(no longer active)]

(In reply to comment #14)
> I don't have time to investigate this at the moment, I think we should back out
> the regressing changeset.  I'll try to prepare a backout soon.

Thanks, a backout seems like a fine option to me.  I hope that the stuff that has landed in the mean time doesn't make your life too difficult!  :-)

Comment 16

[David Zbarsky (:dzbarsky)]

Try run looks ok and this backout fixes the bug.
https://tbpl.mozilla.org/?tree=Try&rev=ca9f6e921798

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a22ba1d1e4

Comment 17

[Carsten Book [:Tomcat]]

(In reply to David Zbarsky (:dzbarsky) from comment #16)
> Try run looks ok and this backout fixes the bug.
> https://tbpl.mozilla.org/?tree=Try&rev=ca9f6e921798
> 
> Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a22ba1d1e4

sorry had to backout this change since it seems this caused b2g build bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=40805281&tree=Mozilla-Inbound

Comment 18

[David Zbarsky (:dzbarsky)]

Full try run: https://tbpl.mozilla.org/?tree=Try&rev=4e2d6f113ded

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b0ccbaa5a4

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This was merged.
https://hg.mozilla.org/mozilla-central/rev/e7b0ccbaa5a4

Comment 20

[Tim Hunt]

Minimal steps to reproduce:

1. Type the word 'Test' into a rich-text editor. (E.g. at http://qa.moodle.net/mod/forum/post.php?forum=2)
2. Type CTRL+Y (Redo)

Expected result: Nothing happens.

Acutal result: It deleted everything you typed. (My actual steps to reproduce involved typing for 20 minutes.)

Please rethink your decision not to put this in Firefox 30 or 31.

Comment 21

[(no longer active)]

I think we should uplift this to Aurora and Beta.  David, do you mind uploading a backout patch and requesting approval please?  Thanks!

Comment 22

[David Zbarsky (:dzbarsky)]

Attachment: Aurora backout

I can't get aurora to build locally but I think this built on try...https://tbpl.mozilla.org/?tree=Try&rev=2c8b281f4803

Comment 23

[David Zbarsky (:dzbarsky)]

Comment on attachment 8443964 [details] [diff] [review]
Aurora backout

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  	828169
User impact if declined: corruption when editing text
Testing completed (on m-c, etc.): original backout fixed regression on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 24

[Tim Hunt]

Thank you for considering my request, and deciding to 'uplift'. I hope these thanks are not just unwanted bugzilla spam.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

David, I see a lot of red on tbpl. Is that caused by something else or your patch? Thanks

Comment 26

[David Zbarsky (:dzbarsky)]

This patch doesn't touch any non-cross platform code, so the fact that it builds on some platforms means it's probably good.  When I pulled from aurora I couldn't get a fresh tree to build locally either, so I'm not really sure what's going on there.  I think this patch is safe, but that's why I asked ehsan for feedback, so he can confirm my judgement here.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8443964 [details] [diff] [review]
Aurora backout

Hmm, about aurora, the bug is marked as landed in 32 (mc) a week ago. However, a week ago, mc was 33.
Ryan, what is a typo from you?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

The bug wasn't marked when it was merged to m-c. My comment was well after the fact. You can confirm that the cset in question is present on Aurora.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8443964 [details] [diff] [review]
Aurora backout

OK. So, it is probably with David could not do anything for aurora.
Waiting for Ehsan before beta uplift.
Thanks guys!

Comment 30

[(no longer active)]

Comment on attachment 8443964 [details] [diff] [review]
Aurora backout

This doesn't build on Aurora as the try push suggests.  Does this build on Beta fine?  It sounds like we don't even need an Aurora backport here.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

David, could you have a look to this bug? We are going to launch the build of beta 5 today.

Comment 32

[David Zbarsky (:dzbarsky)]

I can't build a clean pull of Beta locally so I have no idea if this patch builds.  Are we sure that Beta is affected?  None of the hunks of the backout applied. 

I currently have other obligations so I doubt I'll find time to look into this until late next week.

Comment 33

[(no longer active)]

Sylvestre, can you please get someone test beta to see if it's affected?  Thanks!

Comment 34

[Sylvestre Ledru [:Sylvestre]]

qawanted should be enough! :)

Comment 35

[(no longer active)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #34)
> qawanted should be enough! :)

FWIW in my experience it usually gets ignored until you try to get someone's attention...  Anthony?

Comment 36

[u279076]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #35)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #34)
> > qawanted should be enough! :)
> 
> FWIW in my experience it usually gets ignored until you try to get someone's
> attention...  Anthony?

We're actively watching QAWANTED bugs which are also tracked. If ever in doubt you can need-info [e-mail].

Florin, can you please test this to see if Firefox 31 Beta is affected by this bug?

Comment 37

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I was able to easily reproduce this with a clean profile, in Firefox 31 Beta 7 (BuildID: 20140703154127), on Win 7 x64 ( User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0).

I reproduced the problem with original steps to reproduce from comment 0 and steps from comment 20, slightly modified as follows: 
1. Enter http://qa.moodle.net/mod/forum/post.php?forum=2 and login (e.g. student/test)
2. Type the word 'Test' into the rich-text editor.
3. Copy the word 'Test' then paste it a couple of times.
4. Type CTRL+Y (Redo)

Acutal result: Everything is deleted.

So to me Firefox 31 looks to be affected.

Comment 38

[(no longer active)]

Attachment: Beta backout

https://tbpl.mozilla.org/?tree=Try&rev=d716490281f1

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8451147 [details] [diff] [review]
Beta backout

Seems green. Taking the backout

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c6b3647665e0

Comment 41

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified the fix on Win 7 x64 and Win 8.1 x64 using Firefox 31 Beta 8 (BuildID: 20140707160635) and the latest Firefox 32 Aurora (BuildID: 20140708004001).

I could no longer reproduce the issues from comment 0.

I am though still able to reproduce the issue from comment 20 on http://qa.moodle.net/mod/forum/post.php?forum=2, if I do the following: write "Test", Alt+Tab twice, Ctrl+Y -> text disappears. Note though that this issue also reproduces on the latest Chrome version, so I'm thinking this may be a whole other problem.

David, can you review these results and let me know if this is enough to call this fixed, or should the issue with qa.moodle.net also be fixed here?

Comment 42

[(no longer active)]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #41)
> I've verified the fix on Win 7 x64 and Win 8.1 x64 using Firefox 31 Beta 8
> (BuildID: 20140707160635) and the latest Firefox 32 Aurora (BuildID:
> 20140708004001).
> 
> I could no longer reproduce the issues from comment 0.
> 
> I am though still able to reproduce the issue from comment 20 on
> http://qa.moodle.net/mod/forum/post.php?forum=2, if I do the following:
> write "Test", Alt+Tab twice, Ctrl+Y -> text disappears. Note though that
> this issue also reproduces on the latest Chrome version, so I'm thinking
> this may be a whole other problem.
> 
> David, can you review these results and let me know if this is enough to
> call this fixed, or should the issue with qa.moodle.net also be fixed here?

Can you please get a regression range for comment 20?  It may or may not be the same bug.

Comment 43

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I investigated the case from comment 20, and the issue has been there ever since the Rich Text Editing controls show up in Firefox 15.0.1. On smaller versions, no rich text editing buttons display, so it's just plain text, and Ctrl+Z and Ctrl+Y work without any issues. Given this, I'm inclined to say that this is no recent regression.

Comment 44

[u279076]

Thanks Florin. Given that information I think it's fine to mark this bug verified and comment 20 should be broken out into its own bug report.

Comment 45

[(no longer active)]

Yes, that sounds good.  Thanks!

Comment 46

[Tim Hunt]

Please can someone post the ID of the new bug created as a result of Comment 44. Thanks.

Comment 47

[Lukas Blakk [:lsblakk] use ?needinfo]

This doesn't actually fit ESR criteria and since it's fixed in FF31 (via backout) which is the next ESR version, I'm marking this wontfix on ESR24.