864040 - Enter key isn't displayed correctly with SwiftKey X

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Frederik Niedernolte]

User Agent: Mozilla/5.0 (Android; Mobile; rv:21.0) Gecko/21.0 Firefox/21.0
Build ID: 20130416195807

Steps to reproduce:

Write in a text box like this one on Bugzilla with SwiftKey X on a Nexus 4 with Android 4.2.2 and touch the Enter key. 


Actual results:

Nothing happens, cursor doesn't jump to the next line but when you start to type it will appear correctly in the next line 


Expected results:

The cursor should jump to the next line

Comment 1

[Aaron Train [:aaronmt]]

Frederik, can you try out Nightly for Android (http://nightly.mozilla.org) and report back if that works or doesn't work out for you?

Chris/Jim, is this being tracked in another bug?

Comment 2

[Frederik Niedernolte]

Still same problem with Nightly 23a1

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Set range to after text node when offset is at the end of text node (v1)

When a text node has a newline at the end, if we set the selection range to the end of the text node, the caret will be drawn on the old line. However if we set the selection range to after the text node, the caret will be drawn on the new line.

For example, for <div>test\n</div>, if we set the selection to offset 5 of text node, the caret will be placed on the old line,
> test|
> 
But if we set the selection to offset 1 of div node, the caret will be placed correctly,
> test
> |

This patch detects this case and sets the range to after the text node in all cases.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Send key events from the UI thread the right way (v1)

The code to send key events from the UI thread was never quite right. This patch does it the right way to avoid race conditions. This affects key events such as enter and space.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 765551 [details] [diff] [review]
Send key events from the UI thread the right way (v1)

Review of attachment 765551 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 765549 [details] [diff] [review]
Set range to after text node when offset is at the end of text node (v1)

Um, perhaps, this is an edge case, though:

<p>text\n<strong>strong</strong></p>

If the offset is 5, it seems that your patch sets the offset to before 0 at <strong>. If so, coming text will be inserted into <strong>. Is it your intentional behavior?

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> Comment on attachment 765549 [details] [diff] [review]
> Set range to after text node when offset is at the end of text node (v1)
> 
> Um, perhaps, this is an edge case, though:
> 
> <p>text\n<strong>strong</strong></p>
> 
> If the offset is 5, it seems that your patch sets the offset to before 0 at
> <strong>. If so, coming text will be inserted into <strong>. Is it your
> intentional behavior?

I'm not sure I understand.

If '[]' represent text nodes, and '|' represents the caret, for offset 5 of text node, we have

> <p>[text\n|]<strong>strong</strong></p>

This patch moves to offset 1 of <p> node,

> <p>[text\n]|<strong>strong</strong></p>

The caret is still outside of <strong>.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

If so, it's okay. Could you make automated test for this case? I think you can use nsIDOMWindowUtils.sendSelectionSetEvent() and check the selection range parent.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Set range to after text node when offset is at the end of text node (v2)

The original patch broke runBug722639Test() in widget/tests/window_composition_text_querycontent.xul, and I'm not sure why. This alternate patch does the same thing, but only affects selection events; so query content events are not affected.

I also have a testcase in another patch.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Move synthesizeSelectionSet to EventUtils.js and let sendString accept newlines (v1)

The testcase for this bug is a plain mochitest that needs to use synthesizeSelectionSet from ChromeUtils.js. AFAICT, synthesizeSelectionSet doesn't do anything extra special, and it works just fine if we move it to EventUtils.js.

This patch also makes EventUtils.sendString accept '\n', so that multiline strings can be entered in a single call.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for "Set range to after text node when offset is at the end of text node" (v1)

Testcase for the earlier patch, by comparing the actual caret positions.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 766884 [details] [diff] [review]
Move synthesizeSelectionSet to EventUtils.js and let sendString accept newlines (v1)

Review of attachment 766884 [details] [diff] [review]:
-----------------------------------------------------------------

this appears harmless.  I would like a full test run to ensure that we don't break anything crazy.

::: testing/mochitest/tests/SimpleTest/EventUtils.js
@@ +449,5 @@
>      case '?':
>      case '/':
>        return nsIDOMKeyEvent.DOM_VK_SLASH;
> +    case '\n':
> +      return nsIDOMKeyEvent.DOM_VK_RETURN;

is this \n case not needed in the marionette case?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 766880 [details] [diff] [review]
Set range to after text node when offset is at the end of text node (v2)

Could you swap the if and the else for easier to read?

I mean:

if (node->IsNodeOfType(nsINode::eTEXT)) {
  // The else block's code
} else {
  // The if block's code
}

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Move synthesizeSelectionSet to EventUtils.js and let sendString accept newlines (v2)

(In reply to Joel Maher (:jmaher) from comment #12)
> Comment on attachment 766884 [details] [diff] [review]
> Move synthesizeSelectionSet to EventUtils.js and let sendString accept
> newlines (v1)
> 
> ::: testing/mochitest/tests/SimpleTest/EventUtils.js
> @@ +449,5 @@
> >      case '?':
> >      case '/':
> >        return nsIDOMKeyEvent.DOM_VK_SLASH;
> > +    case '\n':
> > +      return nsIDOMKeyEvent.DOM_VK_RETURN;
> 
> is this \n case not needed in the marionette case?

Good catch; forgot about marionette. This new patch fixes marionette as well.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 767220 [details] [diff] [review]
Move synthesizeSelectionSet to EventUtils.js and let sendString accept newlines (v2)

Review of attachment 767220 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for fixing the b2g stuff.

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for "Set range to after text node when offset is at the end of text node" (v1.1)

Looks like this test still fails intermittently for B2G, so I've disabled it for now. Works great on desktop...

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e31d9c36af
https://hg.mozilla.org/integration/mozilla-inbound/rev/5186929e25c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/af4fda0da7e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5564a3cbd34

Try: https://tbpl.mozilla.org/?tree=Try&rev=cbeb521d8b8b

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c5e31d9c36af
https://hg.mozilla.org/mozilla-central/rev/5186929e25c1
https://hg.mozilla.org/mozilla-central/rev/af4fda0da7e3
https://hg.mozilla.org/mozilla-central/rev/d5564a3cbd34