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

RESOLVED FIXED in Firefox 35

Status

()

Core
Editor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: masayuki, Assigned: Away for a while)

Tracking

({inputmethod, regression})

Trunk
mozilla37
inputmethod, regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox34 wontfix, firefox35+ fixed, firefox36+ fixed, firefox37+ fixed, firefox-esr31 unaffected)

Details

(URL)

Attachments

(1 attachment)

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...
[Tracking Requested - why for this release]: Due to new regression in 37 and dataloss of undo/redo transaction.
tracking-firefox37: --- → ?

Comment 2

3 years ago
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

Updated

3 years ago
Blocks: 1056166
Flags: needinfo?(ayg)

Comment 3

3 years ago
In local build
Last Good: e787a0f1c6f2
First Bad: 8e47222be374

Triggered by 8e47222be374 Aryeh Gregor — Bug 1056166 part 6 - Clean up IMETextTxn; r=ehsan
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox-esr31: --- → unaffected
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?
Keywords: regressionwindow-wanted
tracking-firefox37: ? → +
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.
Flags: needinfo?(ayg)
Regression, tracking.
Ehsan, can you help on this?
status-firefox34: affected → wontfix
tracking-firefox35: ? → +
tracking-firefox36: ? → +
Flags: needinfo?(ehsan.akhgari)
(Assignee)

Comment 6

3 years ago
Masayuki, do you have cycles by any chance?
Flags: needinfo?(ehsan.akhgari) → needinfo?(masayuki)
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.
Flags: needinfo?(masayuki)
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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 9

3 years ago
Bug 1056166 broken the interface maps for IMETextTxn and InsertTextTxn.  I have a fix.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
(Assignee)

Comment 10

3 years ago
Created attachment 8542573 [details] [diff] [review]
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.
(Assignee)

Comment 11

3 years ago
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.
Attachment #8542573 - Flags: approval-mozilla-release?
Attachment #8542573 - Flags: approval-mozilla-beta?
Attachment #8542573 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8542573 - Flags: review?(bugs)
Attachment #8542573 - Flags: review?(bugs) → review+
(Assignee)

Comment 12

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/82953efdb40e
https://hg.mozilla.org/mozilla-central/rev/82953efdb40e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox37: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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.
Attachment #8542573 - Flags: approval-mozilla-release?
Attachment #8542573 - Flags: approval-mozilla-release-
Attachment #8542573 - Flags: approval-mozilla-beta?
Attachment #8542573 - Flags: approval-mozilla-beta+
Attachment #8542573 - Flags: approval-mozilla-aurora?
Attachment #8542573 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 15

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/50f5f0fb4a6e
https://hg.mozilla.org/releases/mozilla-beta/rev/936d2a5ac91b
status-firefox35: affected → fixed
status-firefox36: affected → fixed
(Assignee)

Comment 16

3 years ago
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
(Assignee)

Comment 17

3 years ago
(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 hidden (Treeherder Robot)
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.