Undo/redo corrupts editing text (contenteditable or designmode)

VERIFIED FIXED in Firefox 31

Status

()

defect
--
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: cacyclewp, Assigned: dzbarsky)

Tracking

(Depends on 1 bug, {dataloss, regression})

21 Branch
mozilla32
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 wontfix, firefox30- wontfix, firefox31+ verified, firefox32+ verified, firefox-esr24 wontfix)

Details

Attachments

(4 attachments)

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).
It's almost certainly a regression.
Component: General → Editor
Product: Firefox → Core
Happens in FF 29.0.1 with all add-ons disabled.
(In reply to Cacycle from comment #1)
> It's almost certainly a regression.

Do you have a regression range?
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
Blocks: 828169
Severity: critical → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: dataloss, regression
Version: unspecified → 21 Branch
That patch added some |NS_ENSURE_SUCCESS(result, result);| calls that weren't there previously.  I'll check if one of those is responsible.
Posted patch AttemptSplinter Review
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.
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'
David, can you take this please?
Assignee: nobody → dzbarsky
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.
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 -->
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #8)
> David, can you take this please?

setting flag need info.
Flags: needinfo?(dzbarsky)
(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!
Flags: needinfo?(jst)
Flags: needinfo?(bugs)
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.
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.
Flags: needinfo?(dzbarsky)
(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!  :-)
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
Flags: needinfo?(jst)
Flags: needinfo?(bugs)
(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
This was merged.
https://hg.mozilla.org/mozilla-central/rev/e7b0ccbaa5a4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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.
I think we should uplift this to Aurora and Beta.  David, do you mind uploading a backout patch and requesting approval please?  Thanks!
Flags: needinfo?(dzbarsky)
I can't get aurora to build locally but I think this built on try...https://tbpl.mozilla.org/?tree=Try&rev=2c8b281f4803
Attachment #8443964 - Flags: feedback?(ehsan)
Flags: needinfo?(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
Attachment #8443964 - Flags: approval-mozilla-beta?
Attachment #8443964 - Flags: approval-mozilla-aurora?
Thank you for considering my request, and deciding to 'uplift'. I hope these thanks are not just unwanted bugzilla spam.
David, I see a lot of red on tbpl. Is that caused by something else or your patch? Thanks
Flags: needinfo?(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.
Flags: needinfo?(dzbarsky)
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?
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)
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!
Attachment #8443964 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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.
Attachment #8443964 - Flags: feedback?(ehsan)
Flags: needinfo?(dzbarsky)
David, could you have a look to this bug? We are going to launch the build of beta 5 today.
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.
Flags: needinfo?(dzbarsky)
Sylvestre, can you please get someone test beta to see if it's affected?  Thanks!
Flags: needinfo?(sledru)
Keywords: qawanted
qawanted should be enough! :)
Flags: needinfo?(sledru)
(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?
Flags: needinfo?(anthony.s.hughes)
(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 qa-drivers@mozilla.com.

Florin, can you please test this to see if Firefox 31 Beta is affected by this bug?
Flags: needinfo?(anthony.s.hughes) → needinfo?(florin.mezei)
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.
Flags: needinfo?(florin.mezei)
Keywords: qawanted
Attachment #8443964 - Flags: approval-mozilla-beta?
Comment on attachment 8451147 [details] [diff] [review]
Beta backout

Seems green. Taking the backout
Attachment #8451147 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: checkin-needed
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?
Flags: needinfo?(dzbarsky)
(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.
Flags: needinfo?(dzbarsky) → needinfo?(florin.mezei)
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.
Flags: needinfo?(florin.mezei)
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.
Yes, that sounds good.  Thanks!
Status: RESOLVED → VERIFIED
Keywords: verifyme
Please can someone post the ID of the new bug created as a result of Comment 44. Thanks.
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.
You need to log in before you can comment on or make changes to this bug.