1304624 - Unexpected newline inserted when typing with Chinese input method

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Attachment: screencast

When typing with Chinese input method, unexpected newline could be inserted. This is pretty annoying. (I've actually seen this for quite a while, but didn't file bug because I don't have a highly reliable steps to reproduce.)

Please see the screencast. The input method is Microsoft Pinyin, and the sequence inputed is just "ceshiceshi" and numbers to select candidates.

It seems this happened only on non-e10s.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42b06a7c2f97

Comment 2

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

Okay, this must be a regression of bug 1304625.

STR must be:

1. Activate MS-Pinyin
2. Type something
3. Select all text and remove it
4. Type "ceshiceshi"
5. Commit part of it (E.g., choose "测试")

At step #3, IMEContentObserver notifies TSFTextStore of text changes with wrong range. Therefore, TSFTextStore notifies MS-Pinyin of that and MS-Pinyin caches wrong selection with that. This could cause the other bugs reported by Xidorn.

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=520d53f34fb8

Comment 4

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

Hmm, the patch becomes pretty risky. I should fix bug 1217700 for avoiding regressions and finding similar bugs.

Comment 5

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

Attachment: Bug 1304624 ContentEventHandler::GetFlatTextLengthInRange() shouldn't include a line break length caused by the end node's open tag when the given range ends before the open tag

This must be a regression of bug 1213589.  When removing whole text from an editor, the editor removes moz-<br> after removing all nodes.  Immediately before the remove, moz-<br> becomes <br>.  So, IMEContentObserver::AttributeChanged() detects the native text length for the <br> element changing from 0 to 2 (on Windows) or 1 (on the other platforms).  However, the call of ContentEventHandler::GetFlatTextLengthInRange() in IMEContentObserver::AttributeChanged() returns 2 (on Windows) or 1 (on the other platforms).  Although, it tries to get the length from the start of the editor to before aElement, it always includes the length of the line breaker caused by the end node.

The reason is, ContentEventHandler::GetFlatTextLengthInRange() always adds the line breaker's length for the end node without checking the range includes open tag of the end node.  Therefore, this patch adds the check into ContentEventHandler::GetFlatTextLengthInRange().  So, ContentEventHandler::GetFlatTextLengthInRange() becomes to support NodePositionBefore() for the end of the range.

Additionally, this patch fixes a bug at appending text node length.  In the loop, |aEndPosition| shouldn't be referred because it may be hacked with |endPosition|.  So, detecting the last text node should be compared with |endPosition|.

For making the method code safer, this changes whole |aEndPosition| after defining |endPosition| is replaced with |endPosition|.

Review commit: https://reviewboard.mozilla.org/r/81048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81048/

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8794714 [details]
Bug 1304624 ContentEventHandler::GetFlatTextLengthInRange() shouldn't include a line break length caused by the end node's open tag when the given range ends before the open tag

https://reviewboard.mozilla.org/r/81048/#review80400

(tried to understand this, but didn't yet.)

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8794714 [details]
Bug 1304624 ContentEventHandler::GetFlatTextLengthInRange() shouldn't include a line break length caused by the end node's open tag when the given range ends before the open tag

https://reviewboard.mozilla.org/r/81048/#review80448

Any chance for some tests for all this?

::: dom/events/ContentEventHandler.cpp:2737
(Diff revision 1)
>      // we need to include a line break caused by the open tag.
> -    NodePosition endPosition;
> -    if (aEndPosition.mNode != aRootContent &&
> -        aEndPosition.IsImmediatelyAfterOpenTag()) {
> +    if (endPosition.mNode != aRootContent &&
> +        endPosition.IsImmediatelyAfterOpenTag()) {
> +      if (endPosition.mNode->HasChildren()) {
> -      if (aEndPosition.mNode->HasChildren()) {
>          // When the end node has some children, move the end position to the

Don't you need to modify this comment now that we're using NodePositionBefore, and not NodePosition

::: dom/events/ContentEventHandler.cpp:2745
(Diff revision 1)
>          if (NS_WARN_IF(!firstChild)) {
>            return NS_ERROR_FAILURE;
>          }
> -        endPosition = NodePosition(firstChild, 0);
> +        endPosition = NodePositionBefore(firstChild, 0);
>        } else {
>          // When the end node is empty, move the end position after the node.

(This comment should be still fine.)

::: dom/events/ContentEventHandler.cpp:2805
(Diff revision 1)
>  
>      if (node->IsNodeOfType(nsINode::eTEXT)) {
>        // Note: our range always starts from offset 0
> -      if (node == aEndPosition.mNode) {
> +      if (node == endPosition.mNode) {
>          *aLength += GetTextLength(content, aLineBreakType,
> -                                  aEndPosition.mOffset);
> +                                  endPosition.mOffset);

Aha, this is the endPosition change that the commit message is about. The endPosition stuff above aren't really needed, but just make the code cleaner

Comment 8

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

Comment on attachment 8794714 [details]
Bug 1304624 ContentEventHandler::GetFlatTextLengthInRange() shouldn't include a line break length caused by the end node's open tag when the given range ends before the open tag

https://reviewboard.mozilla.org/r/81048/#review80448

Bug 1217700. I'd like to fix it in Q4 because we need to give up to uplift the patches around IMEContentObserver in a lot of cases due to no automated tests.

> Don't you need to modify this comment now that we're using NodePositionBefore, and not NodePosition

Okay.

(FYI: You can select multiple lines with drag at starting to write comment in MozReview. If you do that like this case, I don't need to check next line's comment.)

> (This comment should be still fine.)

Indeed.

Comment 9

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

Comment on attachment 8794714 [details]
Bug 1304624 ContentEventHandler::GetFlatTextLengthInRange() shouldn't include a line break length caused by the end node's open tag when the given range ends before the open tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81048/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ec418dcac3f
ContentEventHandler::GetFlatTextLengthInRange() shouldn't include a line break length caused by the end node's open tag when the given range ends before the open tag r=smaug

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

(I wanted to ask whether it is upliftable... But I guess since there is no test coverage, it is probably too risky.)

Comment 12

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

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)
> (I wanted to ask whether it is upliftable... But I guess since there is no
> test coverage, it is probably too risky.)

Yes, unfortunately, it's very risky to uplift... The regression cause was also risky, therefore, it was not uplifted too.  Although, riding the train couldn't find this regression before release...

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ec418dcac3f

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

I verified that this is fixed. But I see lots of warnings say:
> [Parent 9116] WARNING: 'mIsInEditAction', file c:/mozilla-source/central/editor/libeditor/EditorBase.cpp, line 1833
which makes typing in my debug build not very fast. Not sure whether that is a problem.

Comment 15

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

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> I verified that this is fixed. But I see lots of warnings say:
> > [Parent 9116] WARNING: 'mIsInEditAction', file c:/mozilla-source/central/editor/libeditor/EditorBase.cpp, line 1833
> which makes typing in my debug build not very fast. Not sure whether that is
> a problem.

Yeah, I usually see the warnings. But I've not found the cause yet.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

It seems I can still reproduce this issue on Twitter with Firefox 52.0b1. I'll investigate a little bit.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Filed bug 1335271.