Unexpected newline inserted when typing with Chinese input method

VERIFIED FIXED in Firefox 52

Status

()

defect
VERIFIED FIXED
3 years ago
4 months ago

People

(Reporter: xidorn, Assigned: masayuki)

Tracking

({inputmethod, regression})

Trunk
mozilla52
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments)

Posted video 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.
See Also: → 1304625
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.
Assignee: nobody → masayuki
Blocks: 1213589
Status: NEW → ASSIGNED
Hardware: Unspecified → All
Hmm, the patch becomes pretty risky. I should fix bug 1217700 for avoiding regressions and finding similar bugs.
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 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
Attachment #8794714 - Flags: review?(bugs) → review+
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.
Pushed by masayuki@d-toybox.com:
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
(I wanted to ask whether it is upliftable... But I guess since there is no test coverage, it is probably too risky.)
(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...
https://hg.mozilla.org/mozilla-central/rev/9ec418dcac3f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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.
Status: RESOLVED → VERIFIED
(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.
Duplicate of this bug: 1305273
It seems I can still reproduce this issue on Twitter with Firefox 52.0b1. I'll investigate a little bit.
Flags: needinfo?(xidorn+moz)
Filed bug 1335271.
Flags: needinfo?(xidorn+moz)
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.