Closed
Bug 1304624
Opened 8 years ago
Closed 8 years ago
Unexpected newline inserted when typing with Chinese input method
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
VERIFIED
FIXED
mozilla52
| Tracking | Status | |
|---|---|---|
| firefox52 | --- | fixed |
People
(Reporter: xidorn, Assigned: masayuki)
References
Details
(Keywords: inputmethod, regression)
Attachments
(2 files)
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.
|
Reporter | |
Updated•8 years ago
|
|
Assignee | |
Updated•8 years ago
|
Keywords: inputmethod
OS: Unspecified → Windows
|
|
Assignee | |
Comment 1•8 years ago
|
||
|
|
Assignee | |
Comment 2•8 years ago
|
||
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 | |
Comment 3•8 years ago
|
||
|
|
Assignee | |
Comment 4•8 years ago
|
||
Hmm, the patch becomes pretty risky. I should fix bug 1217700 for avoiding regressions and finding similar bugs.
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•8 years ago
|
Component: Widget: Win32 → Event Handling
|
|
Assignee | |
Updated•8 years ago
|
Keywords: regression
|
||
Comment 6•8 years ago
|
||
| mozreview-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/#review80400
(tried to understand this, but didn't yet.)
|
|
||
Comment 7•8 years ago
|
||
| mozreview-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
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+
|
|
Assignee | |
Comment 8•8 years ago
|
||
| mozreview-review-reply | ||
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 hidden (mozreview-request) |
|
||
Comment 10•8 years ago
|
||
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
|
|
Reporter | |
Comment 11•8 years ago
|
||
(I wanted to ask whether it is upliftable... But I guess since there is no test coverage, it is probably too risky.)
|
|
Assignee | |
Comment 12•8 years ago
|
||
(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•8 years ago
|
||
| bugherder | ||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
|
|
Reporter | |
Comment 14•8 years ago
|
||
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
|
|
Assignee | |
Comment 15•8 years ago
|
||
(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.
|
|
Reporter | |
Comment 17•8 years ago
|
||
It seems I can still reproduce this issue on Twitter with Firefox 52.0b1. I'll investigate a little bit.
Flags: needinfo?(xidorn+moz)
|
||
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•