1109465 - Committing text with IME at an end of line causes odd undo transactions

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

1. Load the URL.
2. Type Foo for first line text.
3. Type Enter for line break.
4. Type Bar for second line text.
5. Move caret to the end of first line.
6. Use IME and commit a character.
7. Do Undo.

Actual result:
The value becomes "Fooar"

Expected result:
Should be "Foo\nBar"

Aurora doesn't have this bug...

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

[Tracking Requested - why for this release]: Due to new regression in 37 and dataloss of undo/redo transaction.

Comment 2

[Alice0775 White]

hum, I can reproduce the problem on Aurora36.0a2 as well as Firefox34, but not on Firefox33.


Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0461ce2e137
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140829030900
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e47222be374
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140829044401
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e0461ce2e137&tochange=8e47222be374

Regressed by: Bug 1056166

Comment 3

[Alice0775 White]

In local build
Last Good: e787a0f1c6f2
First Bad: 8e47222be374

Triggered by 8e47222be374 Aryeh Gregor — Bug 1056166 part 6 - Clean up IMETextTxn; r=ehsan

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I am not available to do any work until March/April, so if no one else wants to look at this, it might be a good idea to try backing out the patch.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Regression, tracking.
Ehsan, can you help on this?

Comment 6

[(no longer active)]

Masayuki, do you have cycles by any chance?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm very busy for designing new IME API for B2G. However, I'd like to look more next week if I could... If somebody can work on this, feel free to take this.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Ehsan - we're going to build our last desktop beta today but we could take a backout for RC next week if it's low risk.  Can you find time to help with this?  The alternative is to wontfix for 35 since we did already ship but since we know the solution, it would be great to get a fix before release for this recent regression.

Comment 9

[(no longer active)]

Bug 1056166 broken the interface maps for IMETextTxn and InsertTextTxn.  I have a fix.

Comment 10

[(no longer active)]

Attachment: Fix the interface maps of IMETextTxn and InsertTextTxn

The original change in bug 1056166 used NS_INTERFACE_MAP_ENTRY_AMBIGUOUS with
the wrong order for argument names.  But more importantly, we cannot use
NS_INTERFACE_MAP_ENTRY_AMBIGUOUS here because the cast from the concrete
class to nsISupports* will remain ambiguous even when casting first through
nsITransaction, so we need to write the implementation without using any
macros.

Comment 11

[(no longer active)]

Comment on attachment 8542573 [details] [diff] [review]
Fix the interface maps of IMETextTxn and InsertTextTxn

Approval Request Comment
[Feature/regressing bug #]: Bug 1056166.
[User impact if declined]: See comment 0.
[Describe test coverage new/current, TBPL]: Tested locally, and also wrote an automated test.
[Risks and why]: Extremely low risk.  This basically broke QueryInterface() for IMETextTxn and InsertTextTxn, and therefore this code stopped working: <http://dxr.mozilla.org/mozilla-central/source/editor/libeditor/PlaceholderTxn.cpp#125>.  Failing to merge transactions is expected to break undo in the way described in comment 0.
[String/UUID change made/needed]: None.

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82953efdb40e

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/82953efdb40e

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8542573 [details] [diff] [review]
Fix the interface maps of IMETextTxn and InsertTextTxn

No need to put on mozilla-release, we won't chemspill 34 at this stage.

Comment 15

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/50f5f0fb4a6e
https://hg.mozilla.org/releases/mozilla-beta/rev/936d2a5ac91b

Comment 16

[(no longer active)]

The test fails on beta for some reason <https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=936d2a5ac91b> so I disabled it:

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

Comment 17

[(no longer active)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #16)
> The test fails on beta for some reason
> <https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> beta&revision=936d2a5ac91b> so I disabled it:
> 
> https://hg.mozilla.org/releases/mozilla-beta/rev/a6c24971edc2

FWIW I verified the fix on beta manually.

Comment 18

[Legacy TBPL/Treeherder Robot]

submit_timestamp: 2014-12-31T16:29:34
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=mozilla-beta&job_id=201856
repository: mozilla-beta
who: wkocher[at]mozilla[dot]com
machine: tst-linux64-spot-318
buildname: Ubuntu VM 12.04 x64 mozilla-beta pgo test mochitest-4
revision: 936d2a5ac91b

519 INFO TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/tests/test_bug1109465.html | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendCompositionEvent] at chrome://specialpowers/content/specialpowersAPI.js:151