581596 - Use different Android soft keyboards for URL, email, telephone, and search fields

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tony Chung [:tchung]]

The fennec android keyboard isnt showing the correct soft keyboard when entering in the urls.   It's missing the .com and its autocorrecting my URL entry.

per discussion, need to add html5 form work.   Opera Mini and android browser has it right. 

Repro:
1) install android nightly 20100723
2) launch fennec, click the awesomebar
3) verify the soft keyboard is missing .com button, and autocorrects your typing

Expected:
- the right soft keyboard for entering in awesomebar

Comment 2

[Mounir Lamouri (:mounir)]

This bug should not depend on html5forms bug. It should depend on <input type='url'>, <input type='email'>, <input type='tel'>, <input type='color'> (if appropriate) and all date types. HTML5 Forms is far more than that.

Comment 4

[Michael Wu [:mwu]]

Restricting the scope of this bug to currently implemented html5 form types. (email/search/tel/url)

Comment 5

[Michael Wu [:mwu]]

Attachment: 1. Pass input field type to widgets

This is pretty messy. I think widget really just wants to be able to access input element. We can pass even more information to android to setup the keyboard correctly if we know things like whether there is another form element we can tab to. (so the button can say next or submit) Can probably replace some of these bits with a mask containing a number since these input types are all mutually exclusive.

Not sure who I should be asking for review so lemme know if I got it wrong or if we need more people.

Comment 6

[Michael Wu [:mwu]]

Attachment: 2. Make Android use the new input field type data

Straightforward if we're ok with the approach in patch 1.

Comment 7

[Michael Wu [:mwu]]

Attachment: 3. Have fennec set the awesome bar input type to url

For some reason I've seen this create a orange border around the input box. But now I can't reproduce it..

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to comment #6)
> Created attachment 488024 [details] [diff] [review]
> 2. Make Android use the new input field type data
> 
> Straightforward if we're ok with the approach in patch 1.

I think you sent the wrong patch here.

Comment 9

[Michael Wu [:mwu]]

Attachment: 2. Make Android use the new input field type data, v2

Now with more patch.

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to comment #7)
> Created attachment 488025 [details] [diff] [review]
> 3. Have fennec set the awesome bar input type to url
> 
> For some reason I've seen this create a orange border around the input box. But
> now I can't reproduce it..

The orange border is the default style for inputs that fail validation.  (So it should appear when the contents of the urlbar are not a valid URI.)  I think we can override that in CSS using the :invalid pseudo-class.

Comment 11

[Mounir Lamouri (:mounir)]

Can't we try to have a different keyboard even if the type isn't recognized?
I mean, if the author specified <input type='number'>, there will be no number widget but we can have a soft keyboard with numbers only. The same with date and all unsupported types.

To do that, where you do:
+  if (formControl) {
+    PRUint32 type = formControl->GetType();
+    switch (type) {
+    case NS_FORM_INPUT_EMAIL:
+      editorFlags |= nsIPlaintextEditor::eEditorEmailMask;
+      break;
+    case NS_FORM_INPUT_SEARCH:
+      editorFlags |= nsIPlaintextEditor::eEditorSearchMask;
+      break;
+    case NS_FORM_INPUT_TEL:
+      editorFlags |= nsIPlaintextEditor::eEditorTelMask;
+      break;
+    case NS_FORM_INPUT_URL:
+      editorFlags |= nsIPlaintextEditor::eEditorURLMask;
+      break;
+    }

You could add:
default:
  nsCOMPtr<nsIContent> content = do_QueryInterface(mTextCtrlElement);
  nsAutoString type;
  content->GetAttr(kNameSpaceId_None, nsGkAtoms::type, type);
  if (type.EqualsIgnoreCase(NS_LITERAL("number")) {
    editorFlags |= nsIPlaintextEditor::eEditorNumberMask;
  } else /* other types */

I think Safari Mobile is doing that.

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to comment #10)
> (In reply to comment #7)
> > Created attachment 488025 [details] [diff] [review] [details]
> > 3. Have fennec set the awesome bar input type to url
> > 
> > For some reason I've seen this create a orange border around the input box. But
> > now I can't reproduce it..
> 
> The orange border is the default style for inputs that fail validation.  (So it
> should appear when the contents of the urlbar are not a valid URI.)  I think we
> can override that in CSS using the :invalid pseudo-class.

It's red actually. And indeed, you can disable it with:
:invalid {
  box-shadow: none;
}

If you do that, you should have a look at bug 609016 and bug 609017 because they will move the default style from :invalid to :-moz-ui-invalid.

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to comment #11)
> You could add:
> default:
>   nsCOMPtr<nsIContent> content = do_QueryInterface(mTextCtrlElement);
>   nsAutoString type;
>   content->GetAttr(kNameSpaceId_None, nsGkAtoms::type, type);
>   if (type.EqualsIgnoreCase(NS_LITERAL("number")) {
>     editorFlags |= nsIPlaintextEditor::eEditorNumberMask;
>   } else /* other types */

Actually, you should probably check for NS_FORM_INPUT_ELEMENT before doing that (type & NS_FORM_INPUT_ELEMENT) to prevent playing with strings for nothing.

Comment 14

[Michael Wu [:mwu]]

(In reply to comment #11)
> You could add:
> default:
>   nsCOMPtr<nsIContent> content = do_QueryInterface(mTextCtrlElement);
>   nsAutoString type;
>   content->GetAttr(kNameSpaceId_None, nsGkAtoms::type, type);
>   if (type.EqualsIgnoreCase(NS_LITERAL("number")) {
>     editorFlags |= nsIPlaintextEditor::eEditorNumberMask;
>   } else /* other types */
> 
> I think Safari Mobile is doing that.

Ah, interesting! Didn't think of it. Lemme try it..

Comment 15

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

(In reply to comment #4)
> Restricting the scope of this bug to currently implemented html5 form types.
> (email/search/tel/url)

why is that? just the lack of atoms? Also, we may want to use TYPE_TEXT_FLAG_MULTI_LINE for multiline text boxes.

Comment 16

[Michael Wu [:mwu]]

Attachment: 1. Pass input field type to widgets, v2

fix indenting

Comment 17

[Michael Wu [:mwu]]

Attachment: 3. Have fennec set the awesome bar input type to url, v2

eliminate red box-shadow

Comment 18

[Michael Wu [:mwu]]

Attachment: 4. Add support for number inputs

Will request review if patch 1 is ok. Only adding number input support since that's the only other keyboard type that appears to matter for now.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 488106 [details] [diff] [review]
1. Pass input field type to widgets, v2

I don't think that this works fine. You're adding new IME states but why don't you change all widgets on all platforms which support IME states?

> diff --git a/content/base/public/nsIContent.h b/content/base/public/nsIContent.h
> --- a/content/base/public/nsIContent.h
> +++ b/content/base/public/nsIContent.h
> @@ -626,11 +626,17 @@ public:
>      IME_STATUS_PASSWORD = 0x0004,
>      IME_STATUS_PLUGIN   = 0x0008,
>      IME_STATUS_OPEN     = 0x0010,
> -    IME_STATUS_CLOSE    = 0x0020
> +    IME_STATUS_CLOSE    = 0x0020,
> +    IME_STATUS_EMAIL    = 0x0100,
> +    IME_STATUS_SEARCH   = 0x0200,
> +    IME_STATUS_TEL      = 0x0400,
> +    IME_STATUS_URL      = 0x0800,

Please document the details of the new state in above comment block.

> --- a/editor/libeditor/base/nsEditor.cpp
> +++ b/editor/libeditor/base/nsEditor.cpp
> @@ -2063,10 +2063,21 @@ nsEditor::GetPreferredIMEState(PRUint32 

> +  PRUint32 flags = 0;
> +  GetFlags(&flags);
> +
>    switch (frame->GetStyleUIReset()->mIMEMode) {
>      case NS_STYLE_IME_MODE_AUTO:
>        if (IsPasswordEditor())
>          *aState = nsIContent::IME_STATUS_PASSWORD;
> +      else if (flags & nsIPlaintextEditor::eEditorEmailMask)
> +        *aState = nsIContent::IME_STATUS_EMAIL;
> +      else if (flags & nsIPlaintextEditor::eEditorSearchMask)
> +        *aState = nsIContent::IME_STATUS_SEARCH;
> +      else if (flags & nsIPlaintextEditor::eEditorTelMask)
> +        *aState = nsIContent::IME_STATUS_TEL;
> +      else if (flags & nsIPlaintextEditor::eEditorURLMask)
> +        *aState = nsIContent::IME_STATUS_URL;

Why don't you create the similar methods like nsEditor::IsPasswordEditor()? The direct flag check made some mistakes, so, we should create the inline methods.

And please put {} for each if block and else if blocks. That is defined in our coding rules now.

> diff --git a/widget/public/nsIWidget.h b/widget/public/nsIWidget.h
> --- a/widget/public/nsIWidget.h
> +++ b/widget/public/nsIWidget.h
> @@ -1200,7 +1200,15 @@ class nsIWidget : public nsISupports {

> -      IME_STATUS_PLUGIN = 3
> +      IME_STATUS_PLUGIN = 3,
> +      /* Email Field */
> +      IME_STATUS_EMAIL = 4,
> +      /* Search Field */
> +      IME_STATUS_SEARCH = 5,
> +      /* Telephone Field */
> +      IME_STATUS_TEL = 6,
> +      /* URL Field */
> +      IME_STATUS_URL = 7

Why don't you change nsIDOMWindowUtils? And also you must change nsDOMWindowUtils::GetIMEIsOpen() and somewhere. The all new states must make IME enable. So, |state == IME_STATUS_ENABLED| and |state != IME_STATUS_ENABLED| will be changed the meaning. I think that you should create a new static method to nsIWidget such as IsIMEOpenableState(PRUint32 aState).

And please document more things for each status like IME_STATUS_PASSWORD.

And you need to add a lot of tests to http://mxr.mozilla.org/mozilla-central/source/widget/tests/test_imestate.html?force=1

Comment 20

[Michael Wu [:mwu]]

(In reply to comment #19)
> Comment on attachment 488106 [details] [diff] [review]
> 1. Pass input field type to widgets, v2
> 
> I don't think that this works fine. You're adding new IME states but why don't
> you change all widgets on all platforms which support IME states?
> 
I'll update the other widgets. In general, this state is basically the same as IME_STATUS_ENABLED except the widget code has the option of passing hints to the widget so it will be straightforward.

> > --- a/editor/libeditor/base/nsEditor.cpp
> > +++ b/editor/libeditor/base/nsEditor.cpp
> > @@ -2063,10 +2063,21 @@ nsEditor::GetPreferredIMEState(PRUint32 
> 
> > +  PRUint32 flags = 0;
> > +  GetFlags(&flags);
> > +
> >    switch (frame->GetStyleUIReset()->mIMEMode) {
> >      case NS_STYLE_IME_MODE_AUTO:
> >        if (IsPasswordEditor())
> >          *aState = nsIContent::IME_STATUS_PASSWORD;
> > +      else if (flags & nsIPlaintextEditor::eEditorEmailMask)
> > +        *aState = nsIContent::IME_STATUS_EMAIL;
> > +      else if (flags & nsIPlaintextEditor::eEditorSearchMask)
> > +        *aState = nsIContent::IME_STATUS_SEARCH;
> > +      else if (flags & nsIPlaintextEditor::eEditorTelMask)
> > +        *aState = nsIContent::IME_STATUS_TEL;
> > +      else if (flags & nsIPlaintextEditor::eEditorURLMask)
> > +        *aState = nsIContent::IME_STATUS_URL;
> 
> Why don't you create the similar methods like nsEditor::IsPasswordEditor()? The
> direct flag check made some mistakes, so, we should create the inline methods.
> 
Adding another accessor method to this seems to only make things more difficult. IsPasswordEditor is used by other parts of the editor - these new flags are not. The editor will have no reason to access these flags for any purpose other than passing hints along to the IME. What sort of problems do inline methods prevent?

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #20)
> (In reply to comment #19)
> > > --- a/editor/libeditor/base/nsEditor.cpp
> > > +++ b/editor/libeditor/base/nsEditor.cpp
> > > @@ -2063,10 +2063,21 @@ nsEditor::GetPreferredIMEState(PRUint32 
> > 
> > > +  PRUint32 flags = 0;
> > > +  GetFlags(&flags);
> > > +
> > >    switch (frame->GetStyleUIReset()->mIMEMode) {
> > >      case NS_STYLE_IME_MODE_AUTO:
> > >        if (IsPasswordEditor())
> > >          *aState = nsIContent::IME_STATUS_PASSWORD;
> > > +      else if (flags & nsIPlaintextEditor::eEditorEmailMask)
> > > +        *aState = nsIContent::IME_STATUS_EMAIL;
> > > +      else if (flags & nsIPlaintextEditor::eEditorSearchMask)
> > > +        *aState = nsIContent::IME_STATUS_SEARCH;
> > > +      else if (flags & nsIPlaintextEditor::eEditorTelMask)
> > > +        *aState = nsIContent::IME_STATUS_TEL;
> > > +      else if (flags & nsIPlaintextEditor::eEditorURLMask)
> > > +        *aState = nsIContent::IME_STATUS_URL;
> > 
> > Why don't you create the similar methods like nsEditor::IsPasswordEditor()? The
> > direct flag check made some mistakes, so, we should create the inline methods.
> > 
> Adding another accessor method to this seems to only make things more
> difficult. IsPasswordEditor is used by other parts of the editor - these new
> flags are not. The editor will have no reason to access these flags for any
> purpose other than passing hints along to the IME. What sort of problems do
> inline methods prevent?

I meant you should add 4 methods for 4 new flags. IsEmailAddressEditor(), IsSearchEditor(), IsTelEditor() and IsURLEditor().

I think that eEditorEmailMask isn't good name. EmailAddress should be better because it's not clear whether the editor for editing email body (like Thunderbird) or the address.

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to comment #18)
> Created attachment 488109 [details] [diff] [review]
> 4. Add support for number inputs
> 
> Will request review if patch 1 is ok. Only adding number input support since
> that's the only other keyboard type that appears to matter for now.

<input type='number'> and <input type='range'> might both need numbers for the keyboard.
And what about TYPE_CLASS_DATETIME and TYPE_TEXT_FLAG_IME_MULTI_LINE (for textarea's)?

Comment 23

[Michael Wu [:mwu]]

(In reply to comment #22)
> <input type='number'> and <input type='range'> might both need numbers for the
> keyboard.
> And what about TYPE_CLASS_DATETIME and TYPE_TEXT_FLAG_IME_MULTI_LINE (for
> textarea's)?

Unfortunately android doesn't provide a way to provide range hints.

TYPE_CLASS_DATETIME can be used, but in practice, I see little difference from TYPE_CLASS_NUMBER.

TYPE_TEXT_FLAG_IME_MULTI_LINE probably doesn't do much, but the single line variant might.

Comment 24

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

(In reply to comment #23)
> (In reply to comment #22)
> > <input type='number'> and <input type='range'> might both need numbers for the
> > keyboard.
> > And what about TYPE_CLASS_DATETIME and TYPE_TEXT_FLAG_IME_MULTI_LINE (for
> > textarea's)?
> 
> Unfortunately android doesn't provide a way to provide range hints.
> 
> TYPE_CLASS_DATETIME can be used, but in practice, I see little difference from
> TYPE_CLASS_NUMBER.
> 
> TYPE_TEXT_FLAG_IME_MULTI_LINE probably doesn't do much, but the single line
> variant might.

both of these are obviously dependent on the keyboard impl. Its possible to have specific data and time editors.

Also just wanted to re-iterate that I think we should turn on suggestions for multiline text boxes.

Comment 25

[Mounir Lamouri (:mounir)]

(In reply to comment #23)
> (In reply to comment #22)
> > <input type='number'> and <input type='range'> might both need numbers for the
> > keyboard.
> > And what about TYPE_CLASS_DATETIME and TYPE_TEXT_FLAG_IME_MULTI_LINE (for
> > textarea's)?
> 
> Unfortunately android doesn't provide a way to provide range hints.

<input type='range'> is only a number where the exact value is not important (a slider instead of a spinner). Having keyboard for a float would be better than nothing. So, I think we should use TYPE_CLASS_NUMBER.

> TYPE_CLASS_DATETIME can be used, but in practice, I see little difference from
> TYPE_CLASS_NUMBER.

That's still better than the default keyboard). And we could imagine that TYPE_CLASS_DATETIME will evolve. I think we should use it.

> TYPE_TEXT_FLAG_IME_MULTI_LINE probably doesn't do much, but the single line
> variant might.

Using TYPE_TEXT_FLAG_IME_MULTI_LINE wouldn't hurt and it might be better / it can evolve.

Comment 26

[Michael Wu [:mwu]]

I may or may not get to these additional field types. If I don't, please file a followup. Deciding what hints to pass to the keyboard is not rocket science so there is no need to comment on what we should do for each type. I have indeed read the android docs on this topic. The problem is getting *any* info about what field we're in is a huge PITA.

Comment 27

[Michael Wu [:mwu]]

Attachment: 1. Pass IME hints to widgets

This adds a more generic hint passing mechanism for opening IMEs. The only hint right now is the input field type, but we should add more later.

Comment 28

[Michael Wu [:mwu]]

Attachment: 2. Make Android use IME open hints

This covers most things. We need an additional hint to set multiline. Another hint we could us is setting whether we're in web content or not.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 488596 [details] [diff] [review]
1. Pass IME hints to widgets

I think that the hints should be bit flags of int, then, we can send two or more hints even if we need to send in the future. And it's clearer what values are sent to the method.

> +    if (aContent) {
> +      aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::type, hints.mType);
> +    }

This is wrong, the aContent may be non-<input> element.

>      /*
> +     * Same as above, but with extra hints for the IME.
> +     * aHints should never be null.
> +     */
> +    NS_IMETHOD SetIMEEnabled(PRUint32 aState, nsIMEHints* aHints) = 0;

I don't think we need this new method. We should change current method.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What happened between comments #26 and #27? I think the previous approach of extending IMEStatus (attachment #488106 [details] [diff] [review]) was the right one.

Comment 31

[Michael Wu [:mwu]]

(In reply to comment #30)
> What happened between comments #26 and #27? I think the previous approach of
> extending IMEStatus (attachment #488106 [details] [diff] [review]) was the right one.

Why do you say that? I thought it was a terrible one. Extending IMEStatus requires us to change at least 6 files whenever we want to add a new type of IMEStatus for widgets to recognize, whereas using nsIMEHints only requires editing 2 files. Additionally, most new input types can be handled without adding additional core code.

I'm not sure what the additional complexity of adding a new IMEStatus gets us. Editor gets to know, but I don't think editor cares. It cares about password vs. non-password, and I'm not sure why it would care about other types.

We also want to be able to pass more than just what type of field we're in, such as a placeholder string that should be displayed when the field is empty (when editing a field in fullscreen/landscape keyboard mode). This is why I pass a class instead of an int or a string - this will let us add new types of hints for IME opening without changing every single widget that implements SetIMEEnabled.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #31)
> Why do you say that?

Because I don't think we should have "IME states the editor cares about" and "IME states the editor doesn't care about" specified in two different ways.

> I thought it was a terrible one. Extending IMEStatus
> requires us to change at least 6 files whenever we want to add a new type of
> IMEStatus for widgets to recognize,

I think that's a separate problem of its own that we should fix instead of working around.

> Additionally, most new input types can be handled without
> adding additional core code.

Core code still has to map the cross-platform widget type/IME state string to some platform-specific API. GeckoSurfaceView is core code.

> We also want to be able to pass more than just what type of field we're in,
> such as a placeholder string that should be displayed when the field is empty
> (when editing a field in fullscreen/landscape keyboard mode). This is why I
> pass a class instead of an int or a string - this will let us add new types of
> hints for IME opening without changing every single widget that implements
> SetIMEEnabled.

I see.

What if we changed IMEStatus from an enum to a struct like so:
    enum IMEMode {
      IME_MODE_NORMAL,
      IME_MODE_PASSWORD,
      IME_MODE_PLUGIN,
      IME_MODE_HTML_INPUT
    };
    struct IMEStatus {
      IMEMode mMode;
      nsIAtom* mHTMLInputType; // HTML5 "input" type attribute or null, if mode is IME_MODE_HTML_INPUT
    };
and had SetIMEEnabled(IMEStatus* aStatus), where passing null disables IME?

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #32)
> (In reply to comment #31)
> > I thought it was a terrible one. Extending IMEStatus
> > requires us to change at least 6 files whenever we want to add a new type of
> > IMEStatus for widgets to recognize,
> 
> I think that's a separate problem of its own that we should fix instead of
> working around.

I think that we shouldn't take any working around fix.

I think that the right way to fix this bug is to add new states to the enum, or to redesign the SetIMEEnabled(). Either way, you should change all platform's widget code.

> > We also want to be able to pass more than just what type of field we're in,
> > such as a placeholder string that should be displayed when the field is empty
> > (when editing a field in fullscreen/landscape keyboard mode). This is why I
> > pass a class instead of an int or a string - this will let us add new types of
> > hints for IME opening without changing every single widget that implements
> > SetIMEEnabled.
> 
> I see.
> 
> What if we changed IMEStatus from an enum to a struct like so:
>     enum IMEMode {
>       IME_MODE_NORMAL,
>       IME_MODE_PASSWORD,
>       IME_MODE_PLUGIN,
>       IME_MODE_HTML_INPUT
>     };
>     struct IMEStatus {
>       IMEMode mMode;
>       nsIAtom* mHTMLInputType; // HTML5 "input" type attribute or null, if mode
> is IME_MODE_HTML_INPUT
>     };
> and had SetIMEEnabled(IMEStatus* aStatus), where passing null disables IME?

I have a suggestion. The word "IME" isn't good name for now because mobile team is using the SetIMEEnabled() method as an event handler of input context changed. Therefore, I think SetIMEEnabled() should be renamed as OnChangeTextInputContext(TextInputContext* aTextInputContext) and, TextInputContext should has the IMEMode enum and additional information like roc's suggestion.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about just SetInputMode etc? OnChangeTextInputContext is a bit wordy :-)

Comment 35

[Michael Wu [:mwu]]

Attachment: 1. Create SetInputMode API

This patch has only been tested on Android and gtk2, so the changes for the other widgets can be ignored since they're still a WIP.

The code ended up being simpler without passing null when the IME is disabled. I didn't see a need for IME_MODE_HTML_INPUT since the input type should be set if that's the case. There are also public interfaces which expose the IMEStatus enum so avoiding any changes there may be helpful at this point for gecko 2.0.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc:

Isn't a better way to add some new IME states into the enum like the first patch? Then, GetIMEEnabled() can return the specified value, therefore, we can test them automatically by using nsIDOMWindowUtils. However, the current approach is that the result is only seen by users and it's computed in the widget, therefore, we cannot test it easily.

Comment 37

[Michael Wu [:mwu]]

(In reply to comment #36)
> Isn't a better way to add some new IME states into the enum like the first
> patch? Then, GetIMEEnabled() can return the specified value, therefore, we can
> test them automatically by using nsIDOMWindowUtils. However, the current
> approach is that the result is only seen by users and it's computed in the
> widget, therefore, we cannot test it easily.

This is pretty hard to test regardless of whether these hints are part of the enum or not. At least on android, we can only determine whether the keyboard is open or not. We can't tell if the keyboard is a numberpad or qwerty keyboard or anything else. These are truly optional hints for the IME, not new IME states that we must switch into.

I think the best we can do for now is manual testing on Android.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      enum IMEStatus mStatus;

Don't need 'enum' here.

+    NS_IMETHOD SetInputMode(struct IMEContext* aContext) = 0;

How about const IMEContext& aContext?

This looks fine.


(In reply to comment #36)
> Isn't a better way to add some new IME states into the enum like the first
> patch? Then, GetIMEEnabled() can return the specified value, therefore, we can
> test them automatically by using nsIDOMWindowUtils. However, the current
> approach is that the result is only seen by users and it's computed in the
> widget, therefore, we cannot test it easily.

We could copy IMEContext somewhere so we can check it from nsIDOMWindowUtils, if we need to.

I think the most important priority is to have simple clean interfaces. Then we test them however we can.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

okay.

Michael:

The actual result, i.e., whether a virtual keyboard is opened correctly or not, isn't important for now. It's hard to test as you said.

However, we should test whether the widget gets correct state or not. You can test it from widget/tests/test_imestate.html. The tests prevent all unexpected changes of editor and content in XP level.

Comment 40

[Michael Wu [:mwu]]

Attachment: 1. Create SetInputMode API, v2

Suggested API changes made, IME test in the chrome mochitests passes locally. Will do another tryserver run if this looks ok.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    struct nsIWidget::IMEContext context;

Lose 'struct' (occurs in various places)

+      nsAutoString   mHTMLInputType;

How about nsCOMPtr<nsIAtom> instead? I believe the values stored in the input element would normally be atoms, so this would avoid copying.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 489949 [details] [diff] [review]
1. Create SetInputMode API, v2

Never mind the atom thing, it's not worth mucking with interfaces to get it. Just fix the unnecessary 'struct' keywords.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, but use nsString instead of nsAutoString. It's not clear to me that nsAutoString is a win here, and it might be a lose.

Comment 44

[Michael Wu [:mwu]]

Attachment: 1. Create SetInputMode API, v3

Unnecessary 'struct's removed, nsAutoString -> nsString.

Comment 45

[Michael Wu [:mwu]]

Attachment: 2. Make Android use new SetInputMode API

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

You must add tests to test_imestate.html and add InputContextHint property to nsIDOMWindowUtils. And each nsWindow (or nsChildView) should store the hint and nsIWidget::GetInputMode() (rename GetIMEEnabled()) should return it. Note that nsChildView doesn't store the current IME mode and computes it at GetIMEEnabled() but this is a bug, please fix it by this change.

nsEditor:
> nsCOMPtr<nsIContent> content = do_QueryInterface(GetRoot());

Don't use GetRoot(). It returns wrong element in HTML editor. Why don't you use focusedContent?

> nsGtkIMModule::SetInputMode

Please log the hint.

Otherwise, look ok.

roc:

I'm afraid that using nsString causes unnecessary memory fragmentation. SetInputMode() is called at every focus move. In other words, it's called huge number of times. I think enum or nsIAtom* is better.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I really don't think it's going to be an issue. We do many orders of magnitude more string manipulation than that every time we load a page.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

thanks, then, I don't have any objections about that.

Comment 49

[Michael Wu [:mwu]]

Attachment: 4. Create GetInputMode API

Changes requested by masayuki in this patch. Two things:

1. Are we allowed to modify the nsIDOMWindowUtils API like this at this point?
2. The types don't seem to get set correctly in designMode. As what we have now is much better than before, I'd like to address that issue in a followup bug. In the meantime, the type tests are set to todo when in designMode.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can GetInputMode actually fail? If not, then just return the context struct as the method result.

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #50)
> Can GetInputMode actually fail? If not, then just return the context struct as
> the method result.

If the platform doesn't implemented the IME state controlling, it should return NS_ERROR_NOT_IMPLEMENTED. It's checked by nsIMEStateManager. However, if nsBaseWidget holds the state and nsBaseWidget always returns "enabled" state, we can do so. It doesn't change the behavior for XP level code.

# I'll check the patch soon.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 490772 [details] [diff] [review]
4. Create GetInputMode API

I *think* that we're not supposed to add to nsIDOMWindowUtils right now, but I'm not sure. Better check.

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 490772 [details] [diff] [review]
4. Create GetInputMode API

>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
>@@ -139,12 +139,12 @@ nsIMEStateManager::OnChangeFocus(nsPresC

>-    if (enabled ==
>+    if (context.mStatus ==
>         nsContentUtils::GetWidgetStatusFromIMEStatus(newEnabledState)) {

Hmm, I think that this isn't correct in logically. E.g., when the focused editor is changed from a URL field to a text field, then, shouldn't update the hint?

I'm not sure whether the <input> is recreated when the <input> type is changed by script. Furthermore, there is bug 559728. I.e., I'm not sure that this makes an actual bug.

>@@ -195,13 +195,13 @@ nsIMEStateManager::UpdateIMEState(PRUint
>   }
> 
>   // Don't update IME state when enabled state isn't actually changed.
>-  PRUint32 currentEnabledState;
>-  nsresult rv = widget->GetIMEEnabled(&currentEnabledState);
>+  nsIWidget::IMEContext context;
>+  nsresult rv = widget->GetInputMode(context);
>   if (NS_FAILED(rv)) {
>     return; // This platform doesn't support controling the IME state.
>   }
>   PRUint32 newEnabledState = aNewIMEState & nsIContent::IME_STATUS_MASK_ENABLED;
>-  if (currentEnabledState ==
>+  if (context.mStatus ==
>         nsContentUtils::GetWidgetStatusFromIMEStatus(newEnabledState)) {
>     return;
>   }

UpdateIMEState() is called when editor flag is changed. So, you can return from here only when the hint is same too.

>+NS_IMETHODIMP
>+nsDOMWindowUtils::GetIMEHTMLInputType(char** aType)
>+{
>+  NS_ENSURE_ARG_POINTER(aType);
>+
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (!widget)
>+    return NS_ERROR_FAILURE;

nit: would you add {} for the if block.

> NS_IMETHODIMP
>diff --git a/dom/interfaces/base/nsIDOMWindowUtils.idl b/dom/interfaces/base/nsIDOMWindowUtils.idl
>--- a/dom/interfaces/base/nsIDOMWindowUtils.idl
>+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl
>@@ -65,7 +65,7 @@ interface nsITransferable;
> interface nsIQueryContentEventResult;
> interface nsIDOMWindow;
> 
>-[scriptable, uuid(def3fbe8-961b-4c8b-8cac-eb6a4e604bb5)]
>+[scriptable, uuid(85fa978a-fc91-4513-9f11-8911e671577f)]
> interface nsIDOMWindowUtils : nsISupports {
> 
>   /**
>@@ -517,6 +517,11 @@ interface nsIDOMWindowUtils : nsISupport
>   readonly attribute unsigned long IMEStatus;
> 
>   /**
>+   * Get the type of the currently focused html input, if any.
>+   */
>+  readonly attribute string IMEHTMLInputType;
>+

How about focusedInputType ?

>diff --git a/editor/libeditor/base/nsEditor.cpp b/editor/libeditor/base/nsEditor.cpp
>--- a/editor/libeditor/base/nsEditor.cpp
>+++ b/editor/libeditor/base/nsEditor.cpp
>@@ -484,8 +484,7 @@ nsEditor::SetFlags(PRUint32 aFlags)
>     if (NS_SUCCEEDED(rv)) {
>       // NOTE: When the enabled state isn't going to be modified, this method
>       // is going to do nothing.
>-      nsCOMPtr<nsIContent> content = do_QueryInterface(GetRoot());
>-      nsIMEStateManager::UpdateIMEState(newState, content);
>+      nsIMEStateManager::UpdateIMEState(newState, focusedContent);
>     }
>   }

I think that you should include this change to this patch. You should change the original patch. This will add unnecessary history to Blame.

>diff --git a/widget/src/cocoa/nsChildView.mm b/widget/src/cocoa/nsChildView.mm
>--- a/widget/src/cocoa/nsChildView.mm
>+++ b/widget/src/cocoa/nsChildView.mm
>@@ -1970,6 +1970,7 @@ NS_IMETHODIMP nsChildView::SetInputMode(
>   NSLog(@"**** SetInputMode mStatus = %d", aContext.mStatus);
> #endif
> 
>+  mIMEContext = aContext;
>   switch (aContext.mStatus) {
>     case nsIWidget::IME_STATUS_ENABLED:
>     case nsIWidget::IME_STATUS_PLUGIN:
>@@ -1990,19 +1991,13 @@ NS_IMETHODIMP nsChildView::SetInputMode(
>   return NS_OK;
> }
> 
>-NS_IMETHODIMP nsChildView::GetIMEEnabled(PRUint32* aState)
>+NS_IMETHODIMP nsChildView::GetInputMode(IMEContext& aContext)
> {
> #ifdef DEBUG_IME
>-  NSLog(@"**** GetIMEEnabled");
>+  NSLog(@"**** GetInputMode");
> #endif
> 
>-  if (mTextInputHandler.IsIMEEnabled()) {
>-    *aState = nsIWidget::IME_STATUS_ENABLED;
>-  } else if (mTextInputHandler.IsASCIICapableOnly()) {
>-    *aState = nsIWidget::IME_STATUS_PASSWORD;
>-  } else {
>-    *aState = nsIWidget::IME_STATUS_DISABLED;
>-  }
>+  aContext = mIMEContext;
>   return NS_OK;
> }

Thank you!

>+    mIMEContext.mStatus = nsIWidget::IME_STATUS_ENABLED;

I think that this should be set by the default constructor of the struct.

>diff --git a/widget/tests/test_imestate.html b/widget/tests/test_imestate.html
>--- a/widget/tests/test_imestate.html
>+++ b/widget/tests/test_imestate.html
>@@ -25,6 +25,13 @@
>   <input type="button"   id="ibutton"/><br/>
>   <input type="image"    id="image" alt="image"/><br/>
> 
>+  <!-- html5 input elements -->
>+  <input type="url"      id="url"/><br/>
>+  <input type="email"    id="email"/><br/>
>+  <input type="search"   id="search"/><br/>
>+  <input type="tel"      id="tel"/><br/>
>+  <input type="number"   id="number"/><br/>
>+
>   <!-- form controls -->
>   <button id="button">button</button><br/>
>   <textarea id="textarea">textarea</textarea><br/>
>@@ -153,8 +160,17 @@ function runBasicTest(aIsEditable, aInDe
>         return;
>       }
>       enabled = gUtils.IMEStatus;
>+      inputtype = gUtils.IMEHTMLInputType;
>       is(enabled, aTest.expectedEnabled,
>          aDescription + ": " + aTest.description + ", wrong enabled state");
>+      if (aTest.expectedType) {
>+        if (!aInDesignMode)
>+          is(inputtype, aTest.expectedType,
>+             aDescription + ": " + aTest.description + ", wrong input type");
>+        else
>+          todo_is(inputtype, aTest.expectedType,
>+                  aDescription + ": " + aTest.description + ", wrong input type");
>+      }

Shouldn't check else block? The property returns empty string is important thing.

And I *think* that the todo isn't correct. Any <input> fields in HTML editor should be inputtable any characters. So, the property should return empty string always in HTML editor. If this reason doesn't make sense for mobile's software keyboard, it means that the hint isn't enough. Probably, we need a hint whether the inputMode is for HTML editor or not. But it shouldn't be important. The designMode behavior should be changed in future because HTML5 defines the behavior should be same as contenteditable editor.

I.e., you should check whether the expectedType is empty or not in designMode.

>@@ -424,6 +467,21 @@ function runTypeChangingTest()
>     { id: "image",
>       type: "image",    expected: gUtils.IME_STATUS_DISABLED,
>       description: "[type=\"image\"]" },
>+    { id: "url",
>+      type: "url",     expected: gUtils.IME_STATUS_ENABLED,
>+      description: "[type=\"url\"]" },
>+    { id: "email",
>+      type: "email",     expected: gUtils.IME_STATUS_ENABLED,
>+      description: "[type=\"email\"]" },
>+    { id: "search",
>+      type: "search",     expected: gUtils.IME_STATUS_ENABLED,
>+      description: "[type=\"search\"]" },
>+    { id: "tel",
>+      type: "tel",     expected: gUtils.IME_STATUS_ENABLED,
>+      description: "[type=\"tel\"]" },
>+    { id: "number",
>+      type: "number",     expected: gUtils.IME_STATUS_ENABLED,
>+      description: "[type=\"number\"]" },
>     { id: "ime_mode_auto",
>       type: "text",     expected: gUtils.IME_STATUS_ENABLED,  imeMode:  true,
>       description: "[type=\"text\"][ime-mode: auto;]" },

Please add testcases for new HTML5 forms with ime-mode.

And please add testcases in runReadonlyChangingTest() too.

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that IMEContext in nsIWidget should be renamed. It may make confuse some developers who know IME (especially developers of Windows). I think that InputContext is better. And then, the member name should be mInputContext.

# I think I should rename nsIMEStateManager in next alpha cycle...

And when you request next review, would you give a URL of tryserver builds? I should check whether the patch doesn't break IME on the 3 platforms of tier-1.

Unfortunately, I'll travel to Tokyo until next Monday. So, I may not be able to review then. But I hope you don't land the patches without my check.

Comment 55

[Michael Wu [:mwu]]

(In reply to comment #54)
> Unfortunately, I'll travel to Tokyo until next Monday. So, I may not be able to
> review then. But I hope you don't land the patches without my check.

Unfortunately, code freeze is Tuesday next week. I would prefer that we stop changing the spec and scope of this bug at some point so we can get something in. Also, I don't think the nsIDOMWindowUtils change is acceptable at this point, and if it isn't, we'll have to wait until after gecko 2.0 to get these tests in.

My SetInputContext patch is quite conservative IMHO. It doesn't really affect affect the behavior of the state enum that other platforms use, tests cover that enum adequately, and we're willing to take a bit more risk with the new hint string on Android if we can't get tests for it yet.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #55)
> (In reply to comment #54)
> > Unfortunately, I'll travel to Tokyo until next Monday. So, I may not be able to
> > review then. But I hope you don't land the patches without my check.
> 
> Unfortunately, code freeze is Tuesday next week. I would prefer that we stop
> changing the spec and scope of this bug at some point so we can get something
> in. Also, I don't think the nsIDOMWindowUtils change is acceptable at this
> point, and if it isn't, we'll have to wait until after gecko 2.0 to get these
> tests in.

I don't think so. Your patch also changes nsIWidget. Why only nsIDOMWindowUtils change isn't acceptable? The change is just *adding* a new property.

> My SetInputContext patch is quite conservative IMHO. It doesn't really affect
> affect the behavior of the state enum that other platforms use, tests cover
> that enum adequately, and we're willing to take a bit more risk with the new
> hint string on Android if we can't get tests for it yet.

If you don't expand the enum for the risk, I hate the reason because it makes the API implementation complicate.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #56)
> I don't think so. Your patch also changes nsIWidget. Why only nsIDOMWindowUtils
> change isn't acceptable? The change is just *adding* a new property.

bsmedberg, what do you think?

> > My SetInputContext patch is quite conservative IMHO. It doesn't really affect
> > affect the behavior of the state enum that other platforms use, tests cover
> > that enum adequately, and we're willing to take a bit more risk with the new
> > hint string on Android if we can't get tests for it yet.
> 
> If you don't expand the enum for the risk, I hate the reason because it makes
> the API implementation complicate.

I agree with Michael on this one. I think this interface is nice.

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #57)
> (In reply to comment #56)
> > > My SetInputContext patch is quite conservative IMHO. It doesn't really affect
> > > affect the behavior of the state enum that other platforms use, tests cover
> > > that enum adequately, and we're willing to take a bit more risk with the new
> > > hint string on Android if we can't get tests for it yet.
> > 
> > If you don't expand the enum for the risk, I hate the reason because it makes
> > the API implementation complicate.
> 
> I agree with Michael on this one. I think this interface is nice.

Especially, the password state is strange under current design. Why does password field have a specific state but the others have only hints?

By this, there are strange input modes, e.g., the state is "password" but hint is "email" for <input type="email" style="ime-mode: disabled;">.

Of course, on current trunk, there are strange state, e.g., the state is "password" but the type isn't "password" for <input type="text" style="ime-mode: disabled;">.

I think that the change will increase this confusion. Current trunk's state is just an IME state. New state's mStatus isn't changed the meaning from current trunk, so it's a computed status, but mHTMLInputType is physical type of the focused editor (i.e., not computed value). And that also has "password" for different meaning.

If we take the latest patch's interface, I think the new struct should be:

struct InputContext {
  PRBool mIsEditable;
  enum {
    IME_STATUS_DISABLED,
    IME_STATUS_ENABLED,
    IME_STATUS_PLUGIN
  };
  PRUint32 mIMEStatus;
  nsString mHTMLInputType;
};

then, I think that we can define the all states simply and clearly.

So, I think that even if we take the current interface, we should refactor same points. I don't think that it's a good thing.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about if we go with what Michael currently has:
+    struct IMEContext {
+      PRUint32 mStatus;
+
+      /* The type of the input if the input is a html input field */
+      nsString mHTMLInputType;
+    };
and specify that mHTMLInputType is empty if mStatus != IME_STATUS_ENABLED? Also specify that mHTMLInputType is never "password" (since IME_STATUS_PASSWORD is used for that case).

I don't want to use "password" for mHTMLInputType for non-HTML password inputs such as XUL.

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> specify that mHTMLInputType is empty if mStatus != IME_STATUS_ENABLED?

I think that even if IME is disabled by ime-mode: disabled;, the software keyboard should be for email if the type of input is "email". How about you?

> I don't want to use "password" for mHTMLInputType for non-HTML password inputs
such as XUL.

Isn't this bug for chrome's editor (i.e., XUL's editor)?

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #60)
> > specify that mHTMLInputType is empty if mStatus != IME_STATUS_ENABLED?
> 
> I think that even if IME is disabled by ime-mode: disabled;, the software
> keyboard should be for email if the type of input is "email". How about you?

I don't know, but I don't think it really matters. If we decide we want to do that, that's fine with the above rules, we can set IME_STATUS_ENABLED and set mHTMLInputType to "email".

> > I don't want to use "password" for mHTMLInputType for non-HTML password inputs
> such as XUL.
> 
> Isn't this bug for chrome's editor (i.e., XUL's editor)?

I'm not sure what you mean.

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #61)
> (In reply to comment #60)
> > > specify that mHTMLInputType is empty if mStatus != IME_STATUS_ENABLED?
> > 
> > I think that even if IME is disabled by ime-mode: disabled;, the software
> > keyboard should be for email if the type of input is "email". How about you?
> 
> I don't know, but I don't think it really matters. If we decide we want to do
> that, that's fine with the above rules, we can set IME_STATUS_ENABLED and set
> mHTMLInputType to "email".

It breaks ime-mode behavior on new HTML5 input fields...

When ime-mode: diseable; is specified, IME should be disabled. But I think that the software keyboard should be for URL's.

> > > I don't want to use "password" for mHTMLInputType for non-HTML password inputs
> > such as XUL.
> > 
> > Isn't this bug for chrome's editor (i.e., XUL's editor)?
> 
> I'm not sure what you mean.

The main issue of this bug is, awesomebar's software keyboard should be for type="url". See comment 0. Isn't it a XUL element?

Comment 63

[Michael Wu [:mwu]]

(In reply to comment #62)
> > > > I don't want to use "password" for mHTMLInputType for non-HTML password inputs
> > > such as XUL.
> > > 
> > > Isn't this bug for chrome's editor (i.e., XUL's editor)?
> > 
> > I'm not sure what you mean.
> 
> The main issue of this bug is, awesomebar's software keyboard should be for
> type="url". See comment 0. Isn't it a XUL element?

The actual thing that gets focused in the xbl binding is a html:input element. http://hg.mozilla.org/mozilla-central/file/19e98c731909/toolkit/content/widgets/textbox.xml#l23

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #62)
> (In reply to comment #61)
> > (In reply to comment #60)
> > > > specify that mHTMLInputType is empty if mStatus != IME_STATUS_ENABLED?
> > > 
> > > I think that even if IME is disabled by ime-mode: disabled;, the software
> > > keyboard should be for email if the type of input is "email". How about you?
> > 
> > I don't know, but I don't think it really matters. If we decide we want to do
> > that, that's fine with the above rules, we can set IME_STATUS_ENABLED and set
> > mHTMLInputType to "email".
> 
> It breaks ime-mode behavior on new HTML5 input fields...
> 
> When ime-mode: diseable; is specified, IME should be disabled. But I think that
> the software keyboard should be for URL's.

OK then, we can allow mHTMLInputType to be non-empty if mStatus is IME_STATUS_ENABLED or IME_STATUS_DISABLED, and let the platform decide how to handle the cases.

> > > > I don't want to use "password" for mHTMLInputType for non-HTML password inputs
> > > such as XUL.
> > > 
> > > Isn't this bug for chrome's editor (i.e., XUL's editor)?
> > 
> > I'm not sure what you mean.
> 
> The main issue of this bug is, awesomebar's software keyboard should be for
> type="url". See comment 0. Isn't it a XUL element?

No, the URL input field is actually an HTML element.

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #64)
> (In reply to comment #62)
> > (In reply to comment #61)
> > > (In reply to comment #60)
> > > > > specify that mHTMLInputType is empty if mStatus != IME_STATUS_ENABLED?
> > > > 
> > > > I think that even if IME is disabled by ime-mode: disabled;, the software
> > > > keyboard should be for email if the type of input is "email". How about you?
> > > 
> > > I don't know, but I don't think it really matters. If we decide we want to do
> > > that, that's fine with the above rules, we can set IME_STATUS_ENABLED and set
> > > mHTMLInputType to "email".
> > 
> > It breaks ime-mode behavior on new HTML5 input fields...
> > 
> > When ime-mode: diseable; is specified, IME should be disabled. But I think that
> > the software keyboard should be for URL's.
> 
> OK then, we can allow mHTMLInputType to be non-empty if mStatus is
> IME_STATUS_ENABLED or IME_STATUS_DISABLED, and let the platform decide how to
> handle the cases.

IME_STATUS_ENABLED or IME_STATUS_PASSWORD. It's current patch's behavior.

After landing the patch, can I refactor the struct as comment 58 myself? If it's allowed, I'll do it and Michael doesn't need to rename the methods of nsIWidget if he hope so. Then, the patch will be smaller and less risky.

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure.

Comment 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although I'm not sure comment #58 is an improvement, so we'll need to review an actual patch to see.

Comment 68

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, but I don't have much time for now. Either way, I'll create a follow up patch after landing Michael's patch of this bug.

I'll +'ing if Michael updates the patch as comment 53.

Comment 69

[Michael Wu [:mwu]]

Attachment: 1. Create SetInputMode API, v4

Some changes from GetInputMode patch moved into this patch.

Comment 70

[Michael Wu [:mwu]]

Attachment: 4. Create GetInputMode API, v2

Tests revised, IMEHTMLInputType->focusedInputType, test modified as requested, {} added to an if block.

Comment 71

[Michael Wu [:mwu]]

(In reply to comment #53)
> Comment on attachment 490772 [details] [diff] [review]
> 4. Create GetInputMode API
> 
> >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
> >@@ -139,12 +139,12 @@ nsIMEStateManager::OnChangeFocus(nsPresC
> 
> >-    if (enabled ==
> >+    if (context.mStatus ==
> >         nsContentUtils::GetWidgetStatusFromIMEStatus(newEnabledState)) {
> 
> Hmm, I think that this isn't correct in logically. E.g., when the focused
> editor is changed from a URL field to a text field, then, shouldn't update the
> hint?
> 
It does update. I think the state switches from disabled to enabled again when switching fields... not sure, but it works.

> I'm not sure whether the <input> is recreated when the <input> type is changed
> by script. Furthermore, there is bug 559728. I.e., I'm not sure that this makes
> an actual bug.
> 
> >@@ -195,13 +195,13 @@ nsIMEStateManager::UpdateIMEState(PRUint
> >   }
> > 
> >   // Don't update IME state when enabled state isn't actually changed.
> >-  PRUint32 currentEnabledState;
> >-  nsresult rv = widget->GetIMEEnabled(&currentEnabledState);
> >+  nsIWidget::IMEContext context;
> >+  nsresult rv = widget->GetInputMode(context);
> >   if (NS_FAILED(rv)) {
> >     return; // This platform doesn't support controling the IME state.
> >   }
> >   PRUint32 newEnabledState = aNewIMEState & nsIContent::IME_STATUS_MASK_ENABLED;
> >-  if (currentEnabledState ==
> >+  if (context.mStatus ==
> >         nsContentUtils::GetWidgetStatusFromIMEStatus(newEnabledState)) {
> >     return;
> >   }
> 
> UpdateIMEState() is called when editor flag is changed. So, you can return from
> here only when the hint is same too.
> 
The hint is determined in SetIMEState though. I see what you mean though, but it seems like this works fine (according to the ime tests) without the hint check, though I'm not sure how. I suppose I could pull out the hint here to check, but it seems cleaner to keep the hint setting/determination in SetIMEState.

Comment 72

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 491068 [details] [diff] [review]
1. Create SetInputMode API, v4

> nsGtkIMModule::SetInputMode(nsWindow* aCaller, const nsIWidget::IMEContext* aContext)

Why not |const nsIWidget::IMEContext& aContext|? nsIWidget::SetInputMode() is using reference, but here is pointer.

Otherwise, looks ok.

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 491069 [details] [diff] [review]
4. Create GetInputMode API, v2

> nsGtkIMModule::GetInputMode(nsIWidget::IMEContext* aContext)

Please use reference instead of pointer.

> test_imestate.html

> if (aTest.expectedType && !aInDesignMode) {
>   ...
> } else if (aInDesignMode) {

Isn't this just else?


Otherwise, looks ok for me.

Please request sr for both patches before landing.

Comment 74

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #71)
> (In reply to comment #53)
> > Comment on attachment 490772 [details] [diff] [review] [details]
> > 4. Create GetInputMode API
> > 
> > >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
> > >@@ -139,12 +139,12 @@ nsIMEStateManager::OnChangeFocus(nsPresC
> > 
> > >-    if (enabled ==
> > >+    if (context.mStatus ==
> > >         nsContentUtils::GetWidgetStatusFromIMEStatus(newEnabledState)) {
> > 
> > Hmm, I think that this isn't correct in logically. E.g., when the focused
> > editor is changed from a URL field to a text field, then, shouldn't update the
> > hint?
> > 
> It does update. I think the state switches from disabled to enabled again when
> switching fields... not sure, but it works.

When editor is disabled, it's not unfocusable, therefore, here doesn't run then.

I think that this has logically wrong but I have no idea for reproducing the problem. So, don't mind for now.

> > >@@ -195,13 +195,13 @@ nsIMEStateManager::UpdateIMEState(PRUint
> > >   }
> > > 
> > >   // Don't update IME state when enabled state isn't actually changed.
> > >-  PRUint32 currentEnabledState;
> > >-  nsresult rv = widget->GetIMEEnabled(&currentEnabledState);
> > >+  nsIWidget::IMEContext context;
> > >+  nsresult rv = widget->GetInputMode(context);
> > >   if (NS_FAILED(rv)) {
> > >     return; // This platform doesn't support controling the IME state.
> > >   }
> > >   PRUint32 newEnabledState = aNewIMEState & nsIContent::IME_STATUS_MASK_ENABLED;
> > >-  if (currentEnabledState ==
> > >+  if (context.mStatus ==
> > >         nsContentUtils::GetWidgetStatusFromIMEStatus(newEnabledState)) {
> > >     return;
> > >   }
> > 
> > UpdateIMEState() is called when editor flag is changed. So, you can return from
> > here only when the hint is same too.
> > 
> The hint is determined in SetIMEState though.

Oh, right. Then, It means that <input> should call this method when type attribute is changed and if editor isn't reinitialized. E.g., changing the type from "email" to "url".

But even if so, this problem is bug 559728.

Comment 75

[Benjamin Smedberg]

I'm not sure what the question is. We are past API freeze, which means that all interface changes are forbidden. I believe that this includes pseudo-interfaces which are accessible via XPCOM, which means nsIWidget. If it is possible, please create nsIWidget_MOZILLA_2_0 and nsIDOMWindowUtils_MOZILLA_2_0. If not, email [e-mail] and ask for a special exception (although I'm not sure it will be granted for this case).

Comment 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #75)
> I believe that this includes pseudo-interfaces
> which are accessible via XPCOM, which means nsIWidget.

How do we know which interfaces are "pseudo-interfaces accessible via XPCOM"? Is that exactly the interfaces that have an IID and inherit from nsISupports?

Comment 77

[Benjamin Smedberg]

And that a binary extension could somehow get a reference to one, yes.

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, just to confirm, we can keep changing nsIFrame willy-nilly?

Comment 79

[Benjamin Smedberg]

Yes. But nsIContent/nsIDocument need to be stable.

Comment 80

[Michael Wu [:mwu]]

Attachment: 1a. Move SetInputMode to nsIWidget_MOZILLA_2_0_BRANCH

This will be merged into patch 1. Separate right now for easier reviewing.

Comment 81

[Michael Wu [:mwu]]

Attachment: 4a. Move GetInputMode to nsIWidget_MOZILLA_2_0_BRANCH

This will also merge with patch 4 for the commit, but is currently separate for review purposes.

I used static casting for getting nsIWidget_MOZILLA_2_0_BRANCH from an nsIWidget. Not sure if that's right or not.

Comment 82

[Michael Wu [:mwu]]

Attachment: 4a. Move GetInputMode to nsIWidget_MOZILLA_2_0_BRANCH, v2

Removed some cruft I left in by mistake.

roc, will you be able to get to this soon or should I find someone else? This should be relatively straightforward. (but ugly)

Comment 83

[Michael Wu [:mwu]]

Attachment: 4a. Move GetInputMode to nsIWidget_MOZILLA_2_0_BRANCH, v3

Sorry for the spam. I am terrible about running hg qref.

Comment 84

[Michael Wu [:mwu]]

Landed the mobile-browser/fennec patch:
http://hg.mozilla.org/mobile-browser/rev/1d33a97b140a

Comment 85

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/674eb3bc24a5 - SetInputMode API
http://hg.mozilla.org/mozilla-central/rev/92bb7d084e2b - Make Android use hints
http://hg.mozilla.org/mozilla-central/rev/6ed1bf8ddeec - GetInputMode API

Comment 86

[[e-mail]]

You made nsBaseWidget inherit from nsIWidget_MOZILLA_2_0_BRANCH but you didn't change its NS_IMPL_ISUPPORTS macro. This means that when masayuki (erroneously, as it happens) wrote nsCOMPtr<nsWindow> mWindow = aWindow; the debug assertion that exists to catch erroneous nsCOMPtr assignments tries to query aWindow to nsIWidget_MOZILLA_2_0_BRANCH (since that's the IID it finds) and fails. Fortunately that's only a debug assertion; the assignment still succeeds.

Comment 87

[[e-mail]]

I wonder whether since you never actually QI anything to nsIWidget_MOZILLA_2_0_BRANCH you might not actually have to give it an IID.

Comment 88

[Catalin Suciu [:csuciu]]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110920
Firefox/9.0a1 Fennec/9.0a1
Device: HTC Desire
OS: Android 2.2