Closed
Bug 1015433
Opened 10 years ago
Closed 10 years ago
Undo/redo corrupts editing text (contenteditable or designmode)
Categories
(Core :: DOM: Editor, defect)
Tracking
()
People
(Reporter: cacyclewp, Assigned: dzbarsky)
References
(Depends on 1 open bug)
Details
(Keywords: dataloss, regression)
Attachments
(4 files)
2.16 KB,
text/html
|
Details | |
5.32 KB,
patch
|
Details | Diff | Splinter Review | |
56.74 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
54.54 KB,
patch
|
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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).
Updated•10 years ago
|
Component: General → Editor
Product: Firefox → Core
Comment 3•10 years ago
|
||
(In reply to Cacycle from comment #1)
> It's almost certainly a regression.
Do you have a regression range?
Comment 4•10 years ago
|
||
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
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox-esr24:
--- → affected
Ever confirmed: true
Keywords: dataloss,
regression
Version: unspecified → 21 Branch
Assignee | ||
Comment 5•10 years ago
|
||
That patch added some |NS_ENSURE_SUCCESS(result, result);| calls that weren't there previously. I'll check if one of those is responsible.
Assignee | ||
Comment 6•10 years ago
|
||
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•10 years ago
|
||
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'
Updated•10 years ago
|
Comment 9•10 years ago
|
||
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.
Reporter | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
||
(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)
Comment 12•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 14•10 years ago
|
||
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)
Comment 15•10 years ago
|
||
(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! :-)
Assignee | ||
Comment 16•10 years ago
|
||
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)
Comment 17•10 years ago
|
||
(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
Assignee | ||
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
This was merged.
https://hg.mozilla.org/mozilla-central/rev/e7b0ccbaa5a4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 20•10 years ago
|
||
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•10 years ago
|
||
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)
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
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?
Comment 24•10 years ago
|
||
Thank you for considering my request, and deciding to 'uplift'. I hope these thanks are not just unwanted bugzilla spam.
Comment 25•10 years ago
|
||
David, I see a lot of red on tbpl. Is that caused by something else or your patch? Thanks
Flags: needinfo?(dzbarsky)
Assignee | ||
Comment 26•10 years ago
|
||
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 27•10 years ago
|
||
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)
Comment 28•10 years ago
|
||
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 29•10 years ago
|
||
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 30•10 years ago
|
||
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)
Comment 31•10 years ago
|
||
David, could you have a look to this bug? We are going to launch the build of beta 5 today.
Assignee | ||
Comment 32•10 years ago
|
||
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)
Comment 33•10 years ago
|
||
Sylvestre, can you please get someone test beta to see if it's affected? Thanks!
Flags: needinfo?(sledru)
Comment 35•10 years ago
|
||
(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)
Comment 36•10 years ago
|
||
(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)
Comment 37•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8443964 -
Flags: approval-mozilla-beta?
Comment 38•10 years ago
|
||
Attachment #8451147 -
Flags: approval-mozilla-beta?
Comment 39•10 years ago
|
||
Comment on attachment 8451147 [details] [diff] [review]
Beta backout
Seems green. Taking the backout
Attachment #8451147 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 40•10 years ago
|
||
Keywords: checkin-needed
Comment 41•10 years ago
|
||
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)
Comment 42•10 years ago
|
||
(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)
Comment 43•10 years ago
|
||
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)
Comment 44•10 years ago
|
||
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•10 years ago
|
||
Yes, that sounds good. Thanks!
Status: RESOLVED → VERIFIED
Keywords: verifyme
Comment 46•10 years ago
|
||
Please can someone post the ID of the new bug created as a result of Comment 44. Thanks.
Comment 47•10 years ago
|
||
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.
Description
•