Closed Bug 864040 Opened 7 years ago Closed 6 years ago

Enter key isn't displayed correctly with SwiftKey X

Categories

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

21 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25

People

(Reporter: f.niedernolte, Assigned: jchen)

Details

Attachments

(4 files, 3 obsolete files)

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
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?
Hardware: Other → ARM
Still same problem with Nightly 23a1
Assignee: nobody → nchen
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #765549 - Flags: review?(masayuki)
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.
Attachment #765551 - Flags: review?(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
Attachment #765551 - Flags: review?(cpeterson) → review+
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?
(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>.
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.
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.
Attachment #765549 - Attachment is obsolete: true
Attachment #765549 - Flags: review?(masayuki)
Attachment #766880 - Flags: review?(masayuki)
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.
Attachment #766884 - Flags: review?(jmaher)
Testcase for the earlier patch, by comparing the actual caret positions.
Attachment #766885 - Flags: review?(masayuki)
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?
Attachment #766884 - Flags: review?(jmaher) → review+
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
}
Attachment #766880 - Flags: review?(masayuki) → review+
(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.
Attachment #766884 - Attachment is obsolete: true
Attachment #767220 - Flags: review?(jmaher)
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.
Attachment #767220 - Flags: review?(jmaher) → review+
Looks like this test still fails intermittently for B2G, so I've disabled it for now. Works great on desktop...
Attachment #766885 - Attachment is obsolete: true
Attachment #767993 - Flags: review+
You need to log in before you can comment on or make changes to this bug.