Closed Bug 581596 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set

Tracking

(fennec2.0b3+)

VERIFIED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: tchung, Assigned: mwu)

References

()

Details

(Whiteboard: [VKB] )

Attachments

(6 files, 14 obsolete files)

1009 bytes, patch
mfinkle
: review+
Details | Diff | Splinter Review
7.47 KB, patch
blassey
: review+
Details | Diff | Splinter Review
19.45 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
41.25 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
7.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
12.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
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
Depends on: 344614
Summary: Fennec Android has the wrong soft keyboard → Use different Android soft keyboards for URL, email, telephone, etc. fields
tracking-fennec: --- → 2.0+
Duplicate of this bug: 589909
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.
Summary: Use different Android soft keyboards for URL, email, telephone, etc. fields → [VKB] Use different Android soft keyboards for URL, email, telephone, etc. fields
Summary: [VKB] Use different Android soft keyboards for URL, email, telephone, etc. fields → Use different Android soft keyboards for URL, email, telephone, etc. fields
Whiteboard: [VKB]
Duplicate of this bug: 606485
Assignee: nobody → mwu
tracking-fennec: 2.0+ → 2.0b3+
Restricting the scope of this bug to currently implemented html5 form types. (email/search/tel/url)
Depends on: 344615, 456229, 557620, 555559
No longer depends on: 344614
Summary: Use different Android soft keyboards for URL, email, telephone, etc. fields → Use different Android soft keyboards for URL, email, telephone, and search fields
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.
Attachment #488022 - Flags: review?(roc)
Attachment #488022 - Flags: review?(masayuki)
Straightforward if we're ok with the approach in patch 1.
Attachment #488024 - Flags: review?(blassey.bugs)
For some reason I've seen this create a orange border around the input box. But now I can't reproduce it..
Attachment #488025 - Flags: review?(mark.finkle)
(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.
Now with more patch.
Attachment #488024 - Attachment is obsolete: true
Attachment #488024 - Flags: review?(blassey.bugs)
Attachment #488026 - Flags: review?(blassey.bugs)
(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.
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.
(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.
(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.
(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..
(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.
fix indenting
Attachment #488022 - Attachment is obsolete: true
Attachment #488106 - Flags: review?(roc)
Attachment #488022 - Flags: review?(roc)
Attachment #488022 - Flags: review?(masayuki)
Attachment #488106 - Flags: review?(masayuki)
eliminate red box-shadow
Attachment #488025 - Attachment is obsolete: true
Attachment #488025 - Flags: review?(mark.finkle)
Attachment #488108 - Flags: review?(mark.finkle)
Attached patch 4. Add support for number inputs (obsolete) — Splinter Review
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 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
Attachment #488106 - Flags: review?(masayuki) → review-
(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?
Attachment #488108 - Flags: review?(mark.finkle) → review+
(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.
Attachment #488106 - Flags: review?(roc)
(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)?
(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.
(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.
(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.
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.
Attachment #488026 - Flags: review?(blassey.bugs)
Attached patch 1. Pass IME hints to widgets (obsolete) — Splinter Review
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.
Attachment #488106 - Attachment is obsolete: true
Attachment #488109 - Attachment is obsolete: true
Attachment #488596 - Flags: review?(roc)
Attachment #488596 - Flags: review?(masayuki)
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.
Attachment #488026 - Attachment is obsolete: true
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.
Attachment #488596 - Flags: review?(masayuki) → review-
What happened between comments #26 and #27? I think the previous approach of extending IMEStatus (attachment #488106 [details] [diff] [review]) was the right one.
(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.
(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?
(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.
How about just SetInputMode etc? OnChangeTextInputContext is a bit wordy :-)
Attached patch 1. Create SetInputMode API (obsolete) — Splinter Review
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.
Attachment #488596 - Attachment is obsolete: true
Attachment #488598 - Attachment is obsolete: true
Attachment #489045 - Flags: review?(roc)
Attachment #489045 - Flags: review?(masayuki)
Attachment #488596 - Flags: review?(roc)
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.
(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.
+      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.
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.
Attached patch 1. Create SetInputMode API, v2 (obsolete) — Splinter Review
Suggested API changes made, IME test in the chrome mochitests passes locally. Will do another tryserver run if this looks ok.
Attachment #489045 - Attachment is obsolete: true
Attachment #489949 - Flags: review?(roc)
Attachment #489949 - Flags: review?(masayuki)
Attachment #489045 - Flags: review?(roc)
Attachment #489045 - Flags: review?(masayuki)
+    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 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.
Attachment #489949 - Flags: review?(roc) → review+
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.
Attached patch 1. Create SetInputMode API, v3 (obsolete) — Splinter Review
Unnecessary 'struct's removed, nsAutoString -> nsString.
Attachment #489949 - Attachment is obsolete: true
Attachment #489949 - Flags: review?(masayuki)
Attachment #489987 - Flags: review?(blassey.bugs)
Attachment #489962 - Flags: review?(masayuki)
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.
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.
thanks, then, I don't have any objections about that.
Attachment #489987 - Flags: review?(blassey.bugs) → review+
Attached patch 4. Create GetInputMode API (obsolete) — Splinter Review
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.
Attachment #490772 - Flags: review?(roc)
Attachment #490772 - Flags: review?(masayuki)
Can GetInputMode actually fail? If not, then just return the context struct as the method result.
(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 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.
Attachment #490772 - Flags: review?(roc) → review+
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.
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.
(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.
(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.
(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.
(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.
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.
> 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)?
(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.
(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?
(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
(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.
(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.
Although I'm not sure comment #58 is an improvement, so we'll need to review an actual patch to see.
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.
Some changes from GetInputMode patch moved into this patch.
Attachment #491068 - Flags: review?(masayuki)
Tests revised, IMEHTMLInputType->focusedInputType, test modified as requested, {} added to an if block.
Attachment #490772 - Attachment is obsolete: true
Attachment #491069 - Flags: review?
Attachment #490772 - Flags: review?(masayuki)
Attachment #491069 - Flags: review? → review?(masayuki)
(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.
Attachment #489962 - Attachment is obsolete: true
Attachment #489962 - Flags: review?(masayuki)
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.
Attachment #491068 - Flags: review?(masayuki) → review+
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.
Attachment #491069 - Flags: review?(masayuki) → review+
(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.
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 release-drivers@mozilla.org and ask for a special exception (although I'm not sure it will be granted for this case).
No longer blocks: 599365
(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?
And that a binary extension could somehow get a reference to one, yes.
OK, just to confirm, we can keep changing nsIFrame willy-nilly?
Yes. But nsIContent/nsIDocument need to be stable.
This will be merged into patch 1. Separate right now for easier reviewing.
Attachment #491411 - Flags: superreview?(roc)
Attachment #491411 - Flags: review?(roc)
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.
Attachment #491413 - Flags: superreview?(roc)
Attachment #491413 - Flags: review?(roc)
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)
Attachment #491413 - Attachment is obsolete: true
Attachment #491937 - Flags: superreview?(roc)
Attachment #491937 - Flags: review?(roc)
Attachment #491413 - Flags: superreview?(roc)
Attachment #491413 - Flags: review?(roc)
Sorry for the spam. I am terrible about running hg qref.
Attachment #491937 - Attachment is obsolete: true
Attachment #491938 - Flags: superreview?(roc)
Attachment #491938 - Flags: review?(roc)
Attachment #491937 - Flags: superreview?(roc)
Attachment #491937 - Flags: review?(roc)
Attachment #491411 - Flags: superreview?(roc)
Attachment #491411 - Flags: superreview+
Attachment #491411 - Flags: review?(roc)
Attachment #491411 - Flags: review+
Attachment #491938 - Flags: superreview?(roc)
Attachment #491938 - Flags: superreview+
Attachment #491938 - Flags: review?(roc)
Attachment #491938 - Flags: review+
Landed the mobile-browser/fennec patch:
http://hg.mozilla.org/mobile-browser/rev/1d33a97b140a
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 614228
Flags: in-testsuite?
Flags: in-litmus?
No longer blocks: 614228
Depends on: 614228
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.
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.
Depends on: 626178
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
Status: RESOLVED → VERIFIED
Depends on: 757399
You need to log in before you can comment on or make changes to this bug.