Closed Bug 1109465 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Editor, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox34 --- wontfix
firefox35 + fixed
firefox36 + fixed
firefox37 + fixed
firefox-esr31 --- unaffected

People

(Reporter: masayuki, Assigned: ehsan)

References

()

Details

(Keywords: inputmethod, regression)

Attachments

(1 file)

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.
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
Blocks: 1056166
Flags: needinfo?(ayg)
In local build
Last Good: e787a0f1c6f2
First Bad: 8e47222be374

Triggered by 8e47222be374 Aryeh Gregor — Bug 1056166 part 6 - Clean up IMETextTxn; r=ehsan
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?
Flags: needinfo?(ehsan.akhgari)
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)
Bug 1056166 broken the interface maps for IMETextTxn and InsertTextTxn.  I have a fix.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
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 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?
Attachment #8542573 - Flags: review?(bugs)
Attachment #8542573 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/82953efdb40e
Status: NEW → RESOLVED
Closed: 5 years ago
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+
(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.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.