618921 - the chat capability of etherpad application doesn't work with fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Matt Evans [:mevans]]

created a etherpad @ www.ietherpad.com, the chat text area on the right allows the user to submit a line of text with the the input of the carriage return as the event to submit the text. 

This doesn't work in fennec, how works fine with the android browser.

Comment 1

[Josh Matthews [:jdm]]

Since the page uses XMLHttpRequest, it would be worth grabbing NSPR logs to see if everything's behaving correctly.

Comment 2

[Tony Chung [:tchung]]

flagging for blocking final

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

can't reproduce, please reopen if you have STR that work on a recent build

Comment 4

[Matt Evans [:mevans]]

1) go to: http://mozqa.ietherpad.com/3
2) enter text into the lower right text box labeled "chat"
3) press return

Nothing happens.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #4)
> 1) go to: http://mozqa.ietherpad.com/3
> 2) enter text into the lower right text box labeled "chat"
> 3) press return
> 
> Nothing happens.

what build? what device? what keyboard?

Comment 6

[Matt Evans [:mevans]]

1. fennec beta3 build 3
2. Samsung Epic
3. Both physical and virtual keyboards are affected

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

works fine for me too on nexus one and samsung galaxy tab. The only problem I see is the textbox is panned out of view. If I pan it back into view, I can see the text being entered and then added to the chat window when I press "enter" key.

At one time, I did see an issue where the text being entered into the textbox was underlined - pressing enter or submit would not post the text the chat window. In that case I had to "commit" the text to the textbox first, then it all worked.

Comment 8

[Alex Pakhotin (:alexp)]

The problem is in the way how we enable the IME for that Etherpad chat field. IMEContext.mActionHint is "next", so we replace Enter with Tab in GeckoSurfaceView.processKeyDown.

Comment 9

[Alex Pakhotin (:alexp)]

Attachment: Fix

Provide "next" action hint only for the form.

Brad, will you review your own change :) or should the component owner have a look?

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 570405 [details] [diff] [review]
Fix

as you said, this is my code. Smaug, can you review?

The idea here is that if there isn't a form, we don't know that there won't be a submit on enter and we don't know that there will be another input item to "tab" to, so we should just revert to having a return key on the keyboard.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 570405 [details] [diff] [review]
Fix

># HG changeset patch
># User Alex Pakhotin <[e-mail]>
># Date 1319843705 25200
># Node ID c280a1785a34b05868bcba92743d8e8a18af023c
># Parent  bdaf276850c67083d78c0b353f0dfdfd3ce4b43d
>Bug 618921 - the chat capability of etherpad application doesn't work with fennec
>
>diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp
>--- a/content/events/src/nsIMEStateManager.cpp
>+++ b/content/events/src/nsIMEStateManager.cpp
>@@ -310,33 +310,39 @@ nsIMEStateManager::SetIMEState(PRUint32 
>       aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::type,
>                         context.mHTMLInputType);
>       aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::moz_action_hint,
>                         context.mActionHint);
> 
>       // if we don't have an action hint and  return won't submit the form use "next"
>       if (context.mActionHint.IsEmpty() && aContent->Tag() == nsGkAtoms::input) {
>         bool willSubmit = false;
>+        bool hasNext = false;
>         nsCOMPtr<nsIFormControl> control(do_QueryInterface(aContent));
>         mozilla::dom::Element* formElement = control->GetFormElement();
>         nsCOMPtr<nsIForm> form;
>         if (control) {
>           // is this a form and does it have a default submit element?
>           if ((form = do_QueryInterface(formElement)) && form->GetDefaultSubmitElement()) {
>             willSubmit = true;
>           // is this an html form and does it only have a single text input element?
>           } else if (formElement && formElement->Tag() == nsGkAtoms::form && formElement->IsHTML() &&
>                      static_cast<nsHTMLFormElement*>(formElement)->HasSingleTextControl()) {
>             willSubmit = true;
>           }
>+          if (formElement && formElement->Tag() == nsGkAtoms::form)
>+            hasNext = true;

if (expr) {
  stmt;
}

Why formElement->Tag() == nsGkAtoms::form ? What else could a form element be?
So, I don't see any need for hasNext. Just use formElement.



>         }
>         context.mActionHint.Assign(willSubmit ? control->GetType() == NS_FORM_INPUT_SEARCH
>                                                   ? NS_LITERAL_STRING("search")
>                                                   : NS_LITERAL_STRING("go")
>-                                              : NS_LITERAL_STRING("next"));
>+                                              : hasNext
>+                                                  ? NS_LITERAL_STRING("next")
>+                                                  : NS_LITERAL_STRING(""));
>+
I guess you need NS_LITERAL_STRING("") here, but in general EmptyString() is preferred.

Comment 12

[Alex Pakhotin (:alexp)]

Attachment: Fix 2

Addressed review comments.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 570431 [details] [diff] [review]
Fix 2

Please push the patch first to tryserver to make sure
EmptyString() works in this case.
If it doesn't, use NS_LITERAL_STRING("")

Comment 14

[Alex Pakhotin (:alexp)]

Thank you for a quick review, Olli.
Pushed to try.

Comment 15

[Alex Pakhotin (:alexp)]

hg.mozilla.org/projects/birch/rev/9f5570d58bca

Comment 16

[Alex Pakhotin (:alexp)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38e445672780

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/38e445672780