Jim Mathies [:jimm] Assignee
	Description • 12 years ago

Noticed this in the current rev. of Win8. In IE, if you bring up the soft keyboard on the address bar, you get a special keyboard where the return is replaced with 'Go' and there's a '.com' button as well.

In metrofx, we currently get the standard soft keyboard.

	Jim Mathies [:jimm] Assignee
	Comment 1 • 12 years ago

http://msdn.microsoft.com/en-us/library/windows/desktop/ms629025%28v=vs.85%29.aspx

	Jim Mathies [:jimm] Assignee
	Comment 2 • 12 years ago

enum list:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms538181%28v=vs.85%29.aspx

	Jim Mathies [:jimm] Assignee
	Comment 3 • 12 years ago

cc'ing the two Neils!

I've been poking around html specs and in xul trying to figure out if we support some sort of a 'role' concept for text inputs. The interface we want to hook up to here allows us to change the context of the soft keyboard on Windows systems, such that it is customized per the type of text entry expected. So for example a url bar would have extra keys ('.com', 'Go' in place of a return key), an email keyboard would have the '@' symbol on the main layout, etc..

We can expose this through an interface and call it directly for chrome text inputs, but I was wondering if there might already be some sort of role system built into xul that we can hook into. If not, maybe we should consider adding something like this? I was thinking some sort of a common attribute that the focus manager could pick up on and call down into widget to set.

Any thoughts or ideas?

	neil@parkwaycc.co.uk
	Comment 4 • 12 years ago

The XUL textbox is a thinnish wrapper around an HTML input element which does all of the hard work. In particular, <textbox type="url"> should translate into an inner <input type="url">. I don't know whether we have any sort of support for that type of HTML input element though.

	Jim Mathies [:jimm] Assignee
	Comment 5 • 12 years ago

I guess we could connect up IS_URL and IS_PASSWORD for starters. I'm curious about changes in the keyboard for roles like IS_SEARCH. I'll experiment around to see what effects these other settings have.

	Jim Mathies [:jimm] Assignee
	Comment 6 • 12 years ago

We don't set this directly, we have to turn on our TSF layer and respond to requests for GUID_PROP_INPUTSCOPE in RequestSupportedAttrs:

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsTextStore.cpp#990

	Neil Deakin
	Comment 7 • 12 years ago

There's an inputmode attribute defined on html:input that indicates which keyboard to use. It only does anything useful on android I think. You could extend it to support other platforms, values and xul:textbox. See bug 746142.

	Jim Mathies [:jimm] Assignee
	Comment 8 • 12 years ago

Ok, so platform already supports what we need here - in nsIMEStateManager related events sent to widget, we pass in an InputContext which has a mHTMLInputType member. This corresponds to the whatwg experimental input types here - 

http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html

From there, we should be able to set the appropriate scope. Also we'll need to enable a pref by default - 'dom.experimental_forms'.

Doesn't loo like we'll need mHTMLInputInputmode unless we want a specific scope that isn't in the current types list.

	Jim Mathies [:jimm] Assignee
	Comment 10 • 12 years ago

Updates.

	Jim Mathies [:jimm] Assignee
	Comment 11 • 12 years ago

better comments and I used AppendLiteral per Neil's suggestion.

	Jim Mathies [:jimm] Assignee
	Comment 12 • 12 years ago

Comment on attachment 657252 [details] [diff] [review]
rev.3

I've been testing this and am finding some issues with the way we've implemented tsf. In particular, I don't think we should be destroying the document manager object every time focus changes. This seems to be causing problems with the soft keyboard which always tries to hide and then show each time you move to a new text edit within the same page. This might also have something to do with our UIA bridge. I need to do some additional investigating.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 13 • 12 years ago

> I don't think we should be destroying the document manager object every time focus changes.

I hope that you file a new bug only for that.

	Jim Mathies [:jimm] Assignee
	Comment 14 • 12 years ago

This work and isn't going to make the preview.

	Jim Mathies [:jimm] Assignee
	Comment 16 • 12 years ago

This seems to be working much more reliably now. I'll post updated patches to both bugs.

	Jim Mathies [:jimm] Assignee
	Comment 21 • 12 years ago

Comment on attachment 698160 [details] [diff] [review]
metro widget patch

This patch fills out MetroWidget to support text store.

	Jim Mathies [:jimm] Assignee
	Comment 22 • 12 years ago

Comment on attachment 698161 [details] [diff] [review]
scopes patch

This adds tsf input scope support for html5 form inputs.

	Masatoshi Kimura [:emk]
	Comment 23 • 12 years ago

Comment on attachment 698160 [details] [diff] [review]
metro widget patch

Nakano-san is a better reviewer.

	Masatoshi Kimura [:emk]
	Comment 24 • 12 years ago

Comment on attachment 698161 [details] [diff] [review]
scopes patch

Ditto. Only one drive-by comment.

>+  } else if (aHTMLInputType.EqualsLiteral("datetime") ||
>+             aHTMLInputType.EqualsLiteral("date") ||
>+             aHTMLInputType.EqualsLiteral("month") ||
>+             aHTMLInputType.EqualsLiteral("week") ||
>+             aHTMLInputType.EqualsLiteral("datetime-local")) {
>+    mInputScopes.AppendElement(IS_DATE_FULLDATE);
>+    mInputScopes.AppendElement(IS_TIME_FULLTIME);

  } else if (aHTMLInputType.EqualsLiteral("datetime") ||
             aHTMLInputType.EqualsLiteral("datetime-local")) {
    mInputScopes.AppendElement(IS_DATE_FULLDATE);
    mInputScopes.AppendElement(IS_TIME_FULLTIME);
  } else if (aHTMLInputType.EqualsLiteral("date") ||
             aHTMLInputType.EqualsLiteral("month") ||
             aHTMLInputType.EqualsLiteral("week")) {
    mInputScopes.AppendElement(IS_DATE_FULLDATE);
?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 25 • 12 years ago

Comment on attachment 698160 [details] [diff] [review]
metro widget patch

>diff --git a/widget/windows/winrt/MetroWidget.cpp b/widget/windows/winrt/MetroWidget.cpp
>+NS_IMETHODIMP
>+MetroWidget::OnIMEFocusChange(bool aFocus)
>+{
>+  nsresult rv = nsTextStore::OnFocusChange(aFocus, this,
>+                                           mInputContext.mIMEState.mEnabled);
>+  if (rv == NS_ERROR_NOT_AVAILABLE)
>+    rv = NS_ERROR_NOT_IMPLEMENTED; // TSF is not enabled, maybe.
>+  return rv;
>+}

Hmm, on Metro mode, TSF must be available. So, I think that returning NS_ERROR_NOT_AVAILABLE is not problem. So, you can remove the if statement and the next line.

>+
>+NS_IMETHODIMP
>+MetroWidget::OnIMETextChange(uint32_t aStart,
>+                          uint32_t aOldEnd,
>+                          uint32_t aNewEnd)

Fix the odd indent.


I'll try to check the next patch tomorrow with actual test.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 26 • 12 years ago

Comment on attachment 698161 [details] [diff] [review]
scopes patch

Quick simple review for now.

>diff --git a/widget/windows/nsTextStore.cpp b/widget/windows/nsTextStore.cpp
>--- a/widget/windows/nsTextStore.cpp
>+++ b/widget/windows/nsTextStore.cpp
>@@ -18,16 +18,110 @@
> #endif
> #include "nsPrintfCString.h"
> #include "WinUtils.h"
> #include "mozilla/Preferences.h"
> 
> using namespace mozilla;
> using namespace mozilla::widget;
> 
>+#ifdef PR_LOGGING
>+PRLogModuleInfo* sTextStoreLog = nullptr;
>+#endif
>+
>+/******************************************************************/
>+/* InputScopeWrapper                                              */
>+/******************************************************************/
>+
>+// InputScope property GUID
>+static const GUID GUID_PROP_INPUTSCOPE = 
>+{ 0x1713dd5a, 0x68e7, 0x4a5b, { 0x9a, 0xf6, 0x59, 0x2a, 0x59, 0x5c, 0x77, 0x8d } };

Let's use same format at defining uuid. I.e.,

static const GUID GUID_PROP_INPUTSCOPE =
  { 0x1713dd5a, 0x68e7, 0x4a5b,
    { 0x9a, 0xf6, 0x59, 0x2a, 0x59, 0x5c, 0x77, 0x8d } };

# Please remove the last whitespace after "=".

>+
>+class InputScopeWrapper MOZ_FINAL : public ITfInputScope
>+{
>+public:
>+  InputScopeWrapper(const nsTArray<InputScope>& aList) :
>+    mRefCnt(1),
>+    mInputScopes(aList)
>+  {
>+    PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>+      ("TSF: 0x%p InputScopeWrapper()", this));
>+  }
>+
>+  STDMETHODIMP_(ULONG) AddRef(void) {
>+    return ++mRefCnt;
>+  }

Please use

STDMETHODIMP_(ULONG) AddRef(void) { return ++mRefCnt; }

or

STDMETHODIMP_(ULONG) AddRef(void)
{
  return ++mRefCnt;
}

for inline methods. Same for GetPhrase() and so on.

>+
>+  STDMETHODIMP QueryInterface(REFIID riid, void** ppv) {
>+    *ppv=NULL;
>+    if ( (IID_IUnknown == riid) || (IID_ITfInputScope == riid) ) {
>+      *ppv = static_cast<ITfInputScope*>(this);
>+    }
>+    if (*ppv) {
>+      AddRef();
>+      return S_OK;
>+    }
>+    return E_NOINTERFACE;
>+  }
>+
>+  STDMETHODIMP_(ULONG) Release(void) {
>+    --mRefCnt;
>+    if (0 != mRefCnt)
>+      return mRefCnt;

Please use {}. And |if (mRefCnt)| looks better.

>+    PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>+      ("TSF: 0x%p InputScopeWrapper::Release() final", this));
>+    delete this;
>+    return 0;
>+  }
>+
>+  STDMETHODIMP GetInputScopes(InputScope **pprgInputScopes, UINT *pcCount) {

"*" should be immediately after the type rather than the argument name.

>+    DWORD count = (mInputScopes.IsEmpty() ? 1 : mInputScopes.Length());
>+
>+    InputScope* pScope = (InputScope*) CoTaskMemAlloc(sizeof(InputScope) * count);
>+    NS_ENSURE_TRUE(pScope, E_OUTOFMEMORY);
>+
>+    if (mInputScopes.IsEmpty()) {
>+      *pScope = IS_DEFAULT;
>+      *pcCount = 1;
>+      *pprgInputScopes = pScope;
>+      return S_OK;
>+    }
>+
>+    *pcCount = 0;
>+
>+    for (int idx = 0; idx < count; idx++) {
>+      *(pScope + idx) = mInputScopes[idx];
>+      (*pcCount)++;
>+    }

This causes:

> nsTextStore.cpp(90) : warning C4018: '<' : signed/unsigned mismatch

I think both count and idx should be uint32_t.

> STDMETHODIMP
> nsTextStore::RequestSupportedAttrs(DWORD dwFlags,
>                                    ULONG cFilterAttrs,
>                                    const TS_ATTRID *paFilterAttrs)
> {
>   PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>-         ("TSF: 0x%p nsTextStore::RequestSupportedAttrs() called "
>-          "but not supported (S_OK)", this));
>+         ("TSF: 0x%p nsTextStore::RequestSupportedAttrs() called.",
>+          this));

Doesn't need the "." since other logs don't put it.

>-  // no attributes defined
>+  // This is a little weird! RequestSupportedAttrs gives us advanced notice
>+  // of a support query via RetrieveRequestedAttrs for a specific attribute.
>+  // RetrieveRequestedAttrs needs to return valid data for all attributes we
>+  // support, but the text service will only want the input scope object
>+  // returned in RetrieveRequestedAttrs if the dwFlags passed in here contains
>+  // TS_ATTR_FIND_WANT_VALUE.
>+  
>+  // Currently we only support GUID_PROP_INPUTSCOPE
>+  mInputScopeDetected = mInputScopeRequested = false;
>+  if (*paFilterAttrs == GUID_PROP_INPUTSCOPE) {
>+    mInputScopeDetected = true;
>+    if (dwFlags & TS_ATTR_FIND_WANT_VALUE) {
>+      mInputScopeRequested = true;
>+    }
>+  }

I'll read the document tomorrow and review the part after here.

>diff --git a/widget/windows/nsTextStore.h b/widget/windows/nsTextStore.h
>@@ -237,16 +240,20 @@ protected:
>   // of the current composing transaction.
>   nsTextEvent*                 mLastDispatchedTextEvent;
>   // The latest composition string which was dispatched by composition update
>   // event.
>   nsString                     mLastDispatchedCompositionString;
>   // Timer for calling ITextStoreACPSink::OnLayoutChange. This is only used
>   // during composing.
>   nsCOMPtr<nsITimer>           mCompositionTimer;
>+  // The input scopes for this context, defaults to IS_DEFAULT.
>+  nsTArray<InputScope>         mInputScopes;
>+  bool                         mInputScopeDetected;
>+  bool                         mInputScopeRequested;

I don't like similar name bool members since it may make developers confused, but it's okay for now.



And I got this error:
> nsTextStore.cpp(1695) : error C2065: 'IS_SEARCH' : undeclared identifier

Perhaps, it's defined in Win8 SDK. We should define it ourselves #ifndef IS_SEARCH.
http://msdn.microsoft.com/en-us/library/windows/desktop/ms538181%28v=vs.85%29.aspx

	Jim Mathies [:jimm] Assignee
	Comment 27 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> Comment on attachment 698161 [details] [diff] [review]
> scopes patch
> 
> Quick simple review for now.
> 
> >diff --git a/widget/windows/nsTextStore.cpp b/widget/windows/nsTextStore.cpp
> >--- a/widget/windows/nsTextStore.cpp
> >+++ b/widget/windows/nsTextStore.cpp
> >@@ -18,16 +18,110 @@
> > #endif
> > #include "nsPrintfCString.h"
> > #include "WinUtils.h"
> > #include "mozilla/Preferences.h"
> > 
> > using namespace mozilla;
> > using namespace mozilla::widget;
> > 
> >+#ifdef PR_LOGGING
> >+PRLogModuleInfo* sTextStoreLog = nullptr;
> >+#endif
> >+
> >+/******************************************************************/
> >+/* InputScopeWrapper                                              */
> >+/******************************************************************/
> >+
> >+// InputScope property GUID
> >+static const GUID GUID_PROP_INPUTSCOPE = 
> >+{ 0x1713dd5a, 0x68e7, 0x4a5b, { 0x9a, 0xf6, 0x59, 0x2a, 0x59, 0x5c, 0x77, 0x8d } };
> 
> Let's use same format at defining uuid. I.e.,
> 
> static const GUID GUID_PROP_INPUTSCOPE =
>   { 0x1713dd5a, 0x68e7, 0x4a5b,
>     { 0x9a, 0xf6, 0x59, 0x2a, 0x59, 0x5c, 0x77, 0x8d } };
> 
> # Please remove the last whitespace after "=".
> 
> >+
> >+class InputScopeWrapper MOZ_FINAL : public ITfInputScope
> >+{
> >+public:
> >+  InputScopeWrapper(const nsTArray<InputScope>& aList) :
> >+    mRefCnt(1),
> >+    mInputScopes(aList)
> >+  {
> >+    PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
> >+      ("TSF: 0x%p InputScopeWrapper()", this));
> >+  }
> >+
> >+  STDMETHODIMP_(ULONG) AddRef(void) {
> >+    return ++mRefCnt;
> >+  }
> 
> Please use
> 
> STDMETHODIMP_(ULONG) AddRef(void) { return ++mRefCnt; }
> 
> or
> 
> STDMETHODIMP_(ULONG) AddRef(void)
> {
>   return ++mRefCnt;
> }
> 
> for inline methods. Same for GetPhrase() and so on.
> 
> >+
> >+  STDMETHODIMP QueryInterface(REFIID riid, void** ppv) {
> >+    *ppv=NULL;
> >+    if ( (IID_IUnknown == riid) || (IID_ITfInputScope == riid) ) {
> >+      *ppv = static_cast<ITfInputScope*>(this);
> >+    }
> >+    if (*ppv) {
> >+      AddRef();
> >+      return S_OK;
> >+    }
> >+    return E_NOINTERFACE;
> >+  }
> >+
> >+  STDMETHODIMP_(ULONG) Release(void) {
> >+    --mRefCnt;
> >+    if (0 != mRefCnt)
> >+      return mRefCnt;
> 
> Please use {}. And |if (mRefCnt)| looks better.
> 
> >+    PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
> >+      ("TSF: 0x%p InputScopeWrapper::Release() final", this));
> >+    delete this;
> >+    return 0;
> >+  }
> >+
> >+  STDMETHODIMP GetInputScopes(InputScope **pprgInputScopes, UINT *pcCount) {
> 
> "*" should be immediately after the type rather than the argument name.
> 
> >+    DWORD count = (mInputScopes.IsEmpty() ? 1 : mInputScopes.Length());
> >+
> >+    InputScope* pScope = (InputScope*) CoTaskMemAlloc(sizeof(InputScope) * count);
> >+    NS_ENSURE_TRUE(pScope, E_OUTOFMEMORY);
> >+
> >+    if (mInputScopes.IsEmpty()) {
> >+      *pScope = IS_DEFAULT;
> >+      *pcCount = 1;
> >+      *pprgInputScopes = pScope;
> >+      return S_OK;
> >+    }
> >+
> >+    *pcCount = 0;
> >+
> >+    for (int idx = 0; idx < count; idx++) {
> >+      *(pScope + idx) = mInputScopes[idx];
> >+      (*pcCount)++;
> >+    }
> 
> This causes:
> 
> > nsTextStore.cpp(90) : warning C4018: '<' : signed/unsigned mismatch
> 
> I think both count and idx should be uint32_t.
> 
> > STDMETHODIMP
> > nsTextStore::RequestSupportedAttrs(DWORD dwFlags,
> >                                    ULONG cFilterAttrs,
> >                                    const TS_ATTRID *paFilterAttrs)
> > {
> >   PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
> >-         ("TSF: 0x%p nsTextStore::RequestSupportedAttrs() called "
> >-          "but not supported (S_OK)", this));
> >+         ("TSF: 0x%p nsTextStore::RequestSupportedAttrs() called.",
> >+          this));
> 
> Doesn't need the "." since other logs don't put it.
> 
> >-  // no attributes defined
> >+  // This is a little weird! RequestSupportedAttrs gives us advanced notice
> >+  // of a support query via RetrieveRequestedAttrs for a specific attribute.
> >+  // RetrieveRequestedAttrs needs to return valid data for all attributes we
> >+  // support, but the text service will only want the input scope object
> >+  // returned in RetrieveRequestedAttrs if the dwFlags passed in here contains
> >+  // TS_ATTR_FIND_WANT_VALUE.
> >+  
> >+  // Currently we only support GUID_PROP_INPUTSCOPE
> >+  mInputScopeDetected = mInputScopeRequested = false;
> >+  if (*paFilterAttrs == GUID_PROP_INPUTSCOPE) {
> >+    mInputScopeDetected = true;
> >+    if (dwFlags & TS_ATTR_FIND_WANT_VALUE) {
> >+      mInputScopeRequested = true;
> >+    }
> >+  }
> 
> I'll read the document tomorrow and review the part after here.

If you can make sense of the docs better than I was able to great, the docs are confusing. FWIW this code works correctly with the win8 soft keyboard.
 

> >+  bool                         mInputScopeDetected;
> >+  bool                         mInputScopeRequested;
> 
> I don't like similar name bool members since it may make developers
> confused, but it's okay for now.

Not a huge fan of the way this turned out, but the logic for RequestSupportedAttrs has strange requirements.


> And I got this error:
> > nsTextStore.cpp(1695) : error C2065: 'IS_SEARCH' : undeclared identifier
> 
> Perhaps, it's defined in Win8 SDK. We should define it ourselves #ifndef
> IS_SEARCH.
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms538181%28v=vs.
> 85%29.aspx

Ah, yes. Sorry. Will touch this up. FWIW, the official m-c sdk will switch to 8.0 after the next merge.

	Jim Mathies [:jimm] Assignee
	Comment 28 • 12 years ago

nits addressed.

	Jim Mathies [:jimm] Assignee
	Comment 29 • 12 years ago

https://hg.mozilla.org/projects/elm/rev/eb9ed873fa6e
https://hg.mozilla.org/projects/elm/rev/d13a3843e44d

I'll land review changes separately or back this out if major changes are needed.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 30 • 12 years ago

Comment on attachment 698667 [details] [diff] [review]
scopes patch v.2

>diff --git a/widget/windows/nsTextStore.cpp b/widget/windows/nsTextStore.cpp
>--- a/widget/windows/nsTextStore.cpp
>+++ b/widget/windows/nsTextStore.cpp
>@@ -18,16 +18,100 @@
> #endif
> #include "nsPrintfCString.h"
> #include "WinUtils.h"
> #include "mozilla/Preferences.h"
> 
> using namespace mozilla;
> using namespace mozilla::widget;
> 
>+#ifdef PR_LOGGING
>+PRLogModuleInfo* sTextStoreLog = nullptr;
>+#endif
>+
>+#define IS_SEARCH (InputScope)50

Please use static_cast<InputScope>(50).

>+
>+/******************************************************************/
>+/* InputScopeWrapper                                              */
>+/******************************************************************/
>+
>+// InputScope property GUID
>+static const GUID GUID_PROP_INPUTSCOPE =
>+  { 0x1713dd5a, 0x68e7, 0x4a5b,
>+    { 0x9a, 0xf6, 0x59, 0x2a, 0x59, 0x5c, 0x77, 0x8d } };
>+
>+class InputScopeWrapper MOZ_FINAL : public ITfInputScope
>+{
>+public:
>+  InputScopeWrapper(const nsTArray<InputScope>& aList) :
>+    mRefCnt(1),
>+    mInputScopes(aList) {
>+    PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>+      ("TSF: 0x%p InputScopeWrapper()", this));
>+  }
>+
>+  STDMETHODIMP_(ULONG) AddRef(void) { return ++mRefCnt; }
>+
>+  STDMETHODIMP_(ULONG) Release(void) {

Put the "{" to the next line at implementing a method.

>+  STDMETHODIMP QueryInterface(REFIID riid, void** ppv) {

Same.

>+  STDMETHODIMP GetInputScopes(InputScope** pprgInputScopes, UINT* pcCount) {

Same.

>@@ -50,18 +134,16 @@ UINT nsTextStore::sFlushTIPInputMessage 
>  * after that, start with:
>  *   "TSF: 0x%p   nsFoo::Bar("
>  * In an internal method, start with following text:
>  *   "TSF: 0x%p   nsFoo::Bar("
>  * When a static method is called, start with following text:
>  *   "TSF: nsFoo::Bar("
>  */
> 
>-PRLogModuleInfo* sTextStoreLog = nullptr;
>-

Could you move the comment which explains the log's format too?

>@@ -1581,26 +1665,76 @@ nsTextStore::InsertEmbedded(DWORD dwFlag
>   PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>          ("TSF: 0x%p nsTextStore::InsertEmbedded() called "
>           "but not supported (E_NOTIMPL)", this));
> 
>   // embedded objects are not supported
>   return E_NOTIMPL;
> }
> 
>+void
>+nsTextStore::SetInputScope(const nsString& aHTMLInputType)
>+{
>+  mInputScopes.Clear();
>+  if (aHTMLInputType.IsEmpty()) {
>+    return;
>+  }
>+  
>+  // http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html
>+  if (aHTMLInputType.EqualsLiteral("url")) {
>+    mInputScopes.AppendElement(IS_URL);
>+  } else if (aHTMLInputType.EqualsLiteral("search")) {
>+    mInputScopes.AppendElement(IS_SEARCH);
>+  } else if (aHTMLInputType.EqualsLiteral("email")) {
>+    mInputScopes.AppendElement(IS_EMAIL_SMTPEMAILADDRESS);
>+  } else if (aHTMLInputType.EqualsLiteral("password")) {
>+    mInputScopes.AppendElement(IS_PASSWORD);
>+  } else if (aHTMLInputType.EqualsLiteral("datetime") ||
>+             aHTMLInputType.EqualsLiteral("datetime-local")) {
>+    mInputScopes.AppendElement(IS_DATE_FULLDATE);
>+    mInputScopes.AppendElement(IS_TIME_FULLTIME);
>+  } else if (aHTMLInputType.EqualsLiteral("date") ||
>+             aHTMLInputType.EqualsLiteral("month") ||
>+             aHTMLInputType.EqualsLiteral("week")) {
>+    mInputScopes.AppendElement(IS_DATE_FULLDATE);
>+  } else if (aHTMLInputType.EqualsLiteral("time")) {
>+    mInputScopes.AppendElement(IS_TIME_FULLTIME);
>+  } else if (aHTMLInputType.EqualsLiteral("tel")) {
>+    mInputScopes.AppendElement(IS_TELEPHONE_FULLTELEPHONENUMBER);
>+    mInputScopes.AppendElement(IS_TELEPHONE_LOCALNUMBER);
>+  } else if (aHTMLInputType.EqualsLiteral("number")) {
>+    mInputScopes.AppendElement(IS_NUMBER);
>+  }
>+}

I'll check this later.

> STDMETHODIMP
> nsTextStore::RequestSupportedAttrs(DWORD dwFlags,
>                                    ULONG cFilterAttrs,
>                                    const TS_ATTRID *paFilterAttrs)
> {
>   PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>-         ("TSF: 0x%p nsTextStore::RequestSupportedAttrs() called "
>-          "but not supported (S_OK)", this));
>+         ("TSF: 0x%p nsTextStore::RequestSupportedAttrs() called",
>+          this));

Could you log the dwFlags value with the constant names?

> 
>-  // no attributes defined
>+  // This is a little weird! RequestSupportedAttrs gives us advanced notice
>+  // of a support query via RetrieveRequestedAttrs for a specific attribute.
>+  // RetrieveRequestedAttrs needs to return valid data for all attributes we
>+  // support, but the text service will only want the input scope object
>+  // returned in RetrieveRequestedAttrs if the dwFlags passed in here contains
>+  // TS_ATTR_FIND_WANT_VALUE.
>+  
>+  // Currently we only support GUID_PROP_INPUTSCOPE
>+  mInputScopeDetected = mInputScopeRequested = false;
>+  if (*paFilterAttrs == GUID_PROP_INPUTSCOPE) {
>+    mInputScopeDetected = true;
>+    if (dwFlags & TS_ATTR_FIND_WANT_VALUE) {
>+      mInputScopeRequested = true;
>+    }
>+  }

I think you need to check with for loop? Looks like the paFilterAttrs is a pointer to an array and the cFilterAttrs is its length. Then, could you log the each TS_ATTRID values with PR_LOG_ALWAYS?

> STDMETHODIMP
> nsTextStore::RequestAttrsAtPosition(LONG acpPos,
>                                     ULONG cFilterAttrs,
>                                     const TS_ATTRID *paFilterAttrs,
>                                     DWORD dwFlags)

I think that we should implement this too. Our editor's input scope doesn't depend on the position. So, we can support this too. Additionally, we can implement RequestAttrsTransitioningAtPosition() too.

>@@ -1661,21 +1795,42 @@ nsTextStore::RetrieveRequestedAttrs(ULON
> {
>   if (!pcFetched || !ulCount || !paAttrVals) {
>     PR_LOG(sTextStoreLog, PR_LOG_ERROR,
>            ("TSF: 0x%p   nsTextStore::RetrieveRequestedAttrs() FAILED due to "
>             "null argument", this));
>     return E_INVALIDARG;
>   }
> 
>+  if (mInputScopeDetected || mInputScopeRequested) {
>+    paAttrVals->idAttr = GUID_PROP_INPUTSCOPE;
>+    paAttrVals->dwOverlapId = 0;
>+    paAttrVals->varValue.vt = VT_EMPTY;
>+    *pcFetched = 1;
>+
>+    if (mInputScopeRequested) {
>+      paAttrVals->varValue.vt = VT_UNKNOWN;
>+      paAttrVals->varValue.punkVal = (IUnknown*) new InputScopeWrapper(mInputScopes);
>+    }
>+
>+    PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>+           ("TSF: 0x%p nsTextStore::RetrieveRequestedAttrs() succeeded: "
>+            "mInputScopeDetected=%d mInputScopeRequested=%d",
>+            this, mInputScopeDetected, mInputScopeRequested));

Use %s and GetBoolName().

>diff --git a/widget/windows/nsTextStore.h b/widget/windows/nsTextStore.h
>--- a/widget/windows/nsTextStore.h
>+++ b/widget/windows/nsTextStore.h
>@@ -96,16 +97,17 @@ public:
>   {
>     if (!sTsfTextStore) return;
>     sTsfTextStore->CommitCompositionInternal(aDiscard);
>   }
> 
>   static void     SetInputContext(const InputContext& aContext)
>   {
>     if (!sTsfTextStore) return;
>+    sTsfTextStore->SetInputScope(aContext.mHTMLInputType);
>     sTsfTextStore->SetInputContextInternal(aContext.mIMEState.mEnabled);
>   }

It might be better if we pass aContext to SetInputContextInternal() and SetInputScope() is called from it.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 31 • 12 years ago

Comment on attachment 698667 [details] [diff] [review]
scopes patch v.2

>+void
>+nsTextStore::SetInputScope(const nsString& aHTMLInputType)
>+{
>+  mInputScopes.Clear();
>+  if (aHTMLInputType.IsEmpty()) {

|| aHTMLInputType.EqualsLiteral("text")?

>+    return;
>+  }
>+  
>+  // http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html
>+  if (aHTMLInputType.EqualsLiteral("url")) {
>+    mInputScopes.AppendElement(IS_URL);
>+  } else if (aHTMLInputType.EqualsLiteral("search")) {
>+    mInputScopes.AppendElement(IS_SEARCH);
>+  } else if (aHTMLInputType.EqualsLiteral("email")) {
>+    mInputScopes.AppendElement(IS_EMAIL_SMTPEMAILADDRESS);
>+  } else if (aHTMLInputType.EqualsLiteral("password")) {
>+    mInputScopes.AppendElement(IS_PASSWORD);


>+  } else if (aHTMLInputType.EqualsLiteral("datetime") ||
>+             aHTMLInputType.EqualsLiteral("datetime-local")) {
>+    mInputScopes.AppendElement(IS_DATE_FULLDATE);
>+    mInputScopes.AppendElement(IS_TIME_FULLTIME);
>+  } else if (aHTMLInputType.EqualsLiteral("date") ||
>+             aHTMLInputType.EqualsLiteral("month") ||
>+             aHTMLInputType.EqualsLiteral("week")) {
>+    mInputScopes.AppendElement(IS_DATE_FULLDATE);
>+  } else if (aHTMLInputType.EqualsLiteral("time")) {
>+    mInputScopes.AppendElement(IS_TIME_FULLTIME);

I'm not sure why do we need the "datetime", "datetime-local", "date", "month", "week" and "time" cases? We have not implemented the control. And if we use two or more text editor for them like WebKit, we should set only one inputscope for focused editor. So, I think that IS_DEFAULT is enough for them.

How do you think about this, Kimura-san?

>+  } else if (aHTMLInputType.EqualsLiteral("tel")) {
>+    mInputScopes.AppendElement(IS_TELEPHONE_FULLTELEPHONENUMBER);
>+    mInputScopes.AppendElement(IS_TELEPHONE_LOCALNUMBER);

Why don't you add IS_TELEPHONE_COUNTRYCODE and IS_TELEPHONE_AREACODE even you add IS_TELEPHONE_LOCALNUMBER?

>+  } else if (aHTMLInputType.EqualsLiteral("number")) {
>+    mInputScopes.AppendElement(IS_NUMBER);

Um, I'm concerned about "e".
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#valid-floating-point-number
But I guess that this may not be a problem...

FYI: Chrome uses only IS_DEFAULT and IS_PASSWORD.
http://mxr.mozilla.org/chromium/source/src/ui/base/ime/win/tsf_input_scope.cc#45

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 32 • 12 years ago

nit: InputScopeImpl is better than InputScopeWrapper since the class is an implementation of the ITfInputScope, not wrapping another implementation.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 33 • 12 years ago

Looks like the patch work fine on both Win8 and WinXP. Although, I don't see any differences on WinXP.

	Jim Mathies [:jimm] Assignee
	Comment 34 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #33)
> Looks like the patch work fine on both Win8 and WinXP. Although, I don't see
> any differences on WinXP.

I wonder if support on xp was only added to Tablet PC edition? Hard to tell from the msdn docs on InputScope.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 35 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #34)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #33)
> > Looks like the patch work fine on both Win8 and WinXP. Although, I don't see
> > any differences on WinXP.
> 
> I wonder if support on xp was only added to Tablet PC edition? Hard to tell
> from the msdn docs on InputScope.

It depends on the TIP implementation. For example, MS-IME on Win8 closes (turns off) when an editor whose input scope is IS_NUMBER or something. However, ATOK 2013 beta on Win8 doesn't do anything with input scope.

	Jim Mathies [:jimm] Assignee
	Comment 36 • 12 years ago

I'm not sure what RequestAttrsTransitioningAtPosition is supposed to do. Return the same data we return for the entire document in RequestSupportedAttrs?

	Jim Mathies [:jimm] Assignee
	Comment 37 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #31)
> I'm not sure why do we need the "datetime", "datetime-local", "date",
> "month", "week" and "time" cases? We have not implemented the control. And
> if we use two or more text editor for them like WebKit, we should set only
> one inputscope for focused editor. So, I think that IS_DEFAULT is enough for
> them.
> 
> How do you think about this, Kimura-san?

On Win8, the soft keyboard gives you a number centric pad for these, which I prefer to the default keyboard, which doesn't have numerics in the main layout.

	Jim Mathies [:jimm] Assignee
	Comment 38 • 12 years ago

Updates per comments, which sit on top of the original scopes patch that is currently on elm.

some specific comments in a follow up.

	Jim Mathies [:jimm] Assignee
	Comment 39 • 12 years ago

> > STDMETHODIMP
> > nsTextStore::RequestAttrsAtPosition(LONG acpPos,
> >                                     ULONG cFilterAttrs,
> >                                     const TS_ATTRID *paFilterAttrs,
> >                                     DWORD dwFlags)
> 
> I think that we should implement this too. Our editor's input scope doesn't
> depend on the position. So, we can support this too. Additionally, we can
> implement RequestAttrsTransitioningAtPosition() too.

I did not have any luck getting this to work. After windows queries via RequestAttrsAtPosition it then queries RetrieveRequestedAttrs which returns paAttrVals. After which Windows never comes back and queries for the InputScope object so input contexts stop working for the soft keyboard.

I'll test this a bit more to see but for the time being I'm leaving it alone.

> >   static void     SetInputContext(const InputContext& aContext)
> >   {
> >     if (!sTsfTextStore) return;
> >+    sTsfTextStore->SetInputScope(aContext.mHTMLInputType);
> >     sTsfTextStore->SetInputContextInternal(aContext.mIMEState.mEnabled);
> >   }
> 
> It might be better if we pass aContext to SetInputContextInternal() and
> SetInputScope() is called from it.

We call SetInputContextInternal from Create, which doesn't have an InputContext, so instead I passed the html input type string in after the enabled flag, and passed an empty string for that via Create.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 40 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #36)
> I'm not sure what RequestAttrsTransitioningAtPosition is supposed to do.
> Return the same data we return for the entire document in
> RequestSupportedAttrs?

I was thinking so. However, I read the document again:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms538446%28v=vs.85%29.aspx
Then, now I think that we should return nothing since our editor only supports same InputScope in the focused editor. So, "transitioning" never happens.

So, if paFilterAttrs includes GUID_PROP_INPUTSCOPE, set mInputScopeDetected true but mInputScopeRequested should be always false.

(In reply to Jim Mathies [:jimm] from comment #37)
> Created attachment 700513 [details]
> numeric keyboard
> 
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #31)
> > I'm not sure why do we need the "datetime", "datetime-local", "date",
> > "month", "week" and "time" cases? We have not implemented the control. And
> > if we use two or more text editor for them like WebKit, we should set only
> > one inputscope for focused editor. So, I think that IS_DEFAULT is enough for
> > them.
> > 
> > How do you think about this, Kimura-san?
> 
> On Win8, the soft keyboard gives you a number centric pad for these, which I
> prefer to the default keyboard, which doesn't have numerics in the main
> layout.

Yeah, but we have NOT supported the controls for now. I worry about that if the input control will be supported with anonymous editor, it may break the compatibility. So, it might be better not to touch them for now. But if the input types are used on some websites, your code may be better.

FYI: date/time related types shouldn't be a simple text editor like <input type="text">. See http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#attr-input-type
Therefore, I guess that websites don't use them for now since Gecko doesn't support them.

(In reply to Jim Mathies [:jimm] from comment #39)
> > > STDMETHODIMP
> > > nsTextStore::RequestAttrsAtPosition(LONG acpPos,
> > >                                     ULONG cFilterAttrs,
> > >                                     const TS_ATTRID *paFilterAttrs,
> > >                                     DWORD dwFlags)
> > 
> > I think that we should implement this too. Our editor's input scope doesn't
> > depend on the position. So, we can support this too. Additionally, we can
> > implement RequestAttrsTransitioningAtPosition() too.
> 
> I did not have any luck getting this to work. After windows queries via
> RequestAttrsAtPosition it then queries RetrieveRequestedAttrs which returns
> paAttrVals. After which Windows never comes back and queries for the
> InputScope object so input contexts stop working for the soft keyboard.

Sorry, I don't understand what you meant at the last sentence. I think that you should make a method which is same as your RequestSupportedAttrs(). And all of the RequestSupportedAttrs, the RequestAttrsAtPosition() and the RequestAttrsTransitioningAtPosition(). And then, only RequestAttrsTransitioningAtPosition() overwrites mInputScopeRequested false.

> I'll test this a bit more to see but for the time being I'm leaving it alone.
> 
> > >   static void     SetInputContext(const InputContext& aContext)
> > >   {
> > >     if (!sTsfTextStore) return;
> > >+    sTsfTextStore->SetInputScope(aContext.mHTMLInputType);
> > >     sTsfTextStore->SetInputContextInternal(aContext.mIMEState.mEnabled);
> > >   }
> > 
> > It might be better if we pass aContext to SetInputContextInternal() and
> > SetInputScope() is called from it.
> 
> We call SetInputContextInternal from Create, which doesn't have an
> InputContext, so instead I passed the html input type string in after the
> enabled flag, and passed an empty string for that via Create.

Ah, I forgot that. Then, I think that your first approach is better (i.e., calling SetInputScope() from nsTextStore::SetInputContext()). Sorry for my mistake.

	Masatoshi Kimura [:emk]
	Comment 41 • 12 years ago

Comment on attachment 698667 [details] [diff] [review]
scopes patch v.2

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #31)
> I'm not sure why do we need the "datetime", "datetime-local", "date",
> "month", "week" and "time" cases? We have not implemented the control. And
> if we use two or more text editor for them like WebKit, we should set only
> one inputscope for focused editor. So, I think that IS_DEFAULT is enough for
> them.
> 
> How do you think about this, Kimura-san?

Some TIPs may filter the candidate based on the input scope.
http://nyaruru.hatenablog.com/entry/20070309/p1

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 42 • 12 years ago

My another question is, do you think that we need to set the date/time related InputScope for them *before* we implement such controls, i.e., now?

	Masatoshi Kimura [:emk]
	Comment 43 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #42)
> My another question is, do you think that we need to set the date/time
> related InputScope for them *before* we implement such controls, i.e., now?

Inputscope-aware-TIPs will suggest only URLs for <input type="url">, dates and times for <input type="datetime">, and so on.
IMO It will improve UX even if we don't show a dedicated input control.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 44 • 12 years ago

I see, thank you, Kimura-san.

	Jim Mathies [:jimm] Assignee
	Comment 45 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #40)
> > > I think that we should implement this too. Our editor's input scope doesn't
> > > depend on the position. So, we can support this too. Additionally, we can
> > > implement RequestAttrsTransitioningAtPosition() too.
> > 
> > I did not have any luck getting this to work. After windows queries via
> > RequestAttrsAtPosition it then queries RetrieveRequestedAttrs which returns
> > paAttrVals. After which Windows never comes back and queries for the
> > InputScope object so input contexts stop working for the soft keyboard.
> 
> Sorry, I don't understand what you meant at the last sentence. I think that
> you should make a method which is same as your RequestSupportedAttrs(). And
> all of the RequestSupportedAttrs, the RequestAttrsAtPosition() and the
> RequestAttrsTransitioningAtPosition(). And then, only
> RequestAttrsTransitioningAtPosition() overwrites mInputScopeRequested false.


This is basically what I did. I'll generate some logs for comparison.

	Jim Mathies [:jimm] Assignee
	Comment 46 • 12 years ago

Here's the log data from the two different methods with some notes. For some reason, Windows doesn't react as expected to RetrieveRequestedAttrs returning an id after RequestAttrsAtPosition.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 47 • 12 years ago

The dwFlags of RequestAttrsAtPosition() is always 0.
http://msdn.microsoft.com/en-us/library/windows/desktop/ms538445%28v=vs.85%29.aspx

However, it doesn't make sense to think that the caller wants to retrieve the data without the value. So, I think that we should think that TS_ATTR_FIND_WANT_VALUE is specified to the dwFlags. In other words, mInputScopeRequested needs to be same value as mInputScopeDetected if RequestAttrsAtPosition() is called.

	Jim Mathies [:jimm] Assignee
	Comment 48 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #47)
> The dwFlags of RequestAttrsAtPosition() is always 0.
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms538445%28v=vs.
> 85%29.aspx
> 
> However, it doesn't make sense to think that the caller wants to retrieve
> the data without the value. So, I think that we should think that
> TS_ATTR_FIND_WANT_VALUE is specified to the dwFlags. In other words,
> mInputScopeRequested needs to be same value as mInputScopeDetected if
> RequestAttrsAtPosition() is called.

I'll test with that. From earlier testing though I remember having a problem where if you return an InputScope object in cases where TS_ATTR_FIND_WANT_VALUE isn't specified, the object leaked. Will recheck this again.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 49 • 12 years ago

Hi, when will you attach the new patch? My patches for bug 790516 depend on this bug's first patch. If you need long time for this bug, I'll rewrite the patches and request the review to Kimura-san.

And I'd like to review your patch for differences with m-c, not elm.

	Jim Mathies [:jimm] Assignee
	Comment 50 • 12 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #49)
> Hi, when will you attach the new patch? My patches for bug 790516 depend on
> this bug's first patch. If you need long time for this bug, I'll rewrite the
> patches and request the review to Kimura-san.
> 
> And I'd like to review your patch for differences with m-c, not elm.

Will get to this this week. Sorry, we had a work week last week that tied me up.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 51 • 12 years ago

Okay, no problem. I wait your patch.

	Jim Mathies [:jimm] Assignee
	Comment 52 • 12 years ago

Ok, here's the updated patch based on mc.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 53 • 12 years ago

Comment on attachment 706332 [details] [diff] [review]
scopes patch

>diff --git a/widget/windows/nsTextStore.cpp b/widget/windows/nsTextStore.cpp
>@@ -52,23 +39,169 @@ UINT nsTextStore::sFlushTIPInputMessage 
>  *   "TSF: 0x%p   nsFoo::Bar("
>  * In an internal method, start with following text:
>  *   "TSF: 0x%p   nsFoo::Bar("
>  * When a static method is called, start with following text:
>  *   "TSF: nsFoo::Bar("
>  */
> 
> PRLogModuleInfo* sTextStoreLog = nullptr;
>+#endif

nit: |#endif // #ifdef PR_LOGGING|

>+class InputScopeWrapper MOZ_FINAL : public ITfInputScope

I think that InputScopeImpl is better name.

>+static const nsCString
>+GetFindFlagName(DWORD aFindFlag)
>+{

>+  if (description.IsEmpty()) {
>+    description.AppendLiteral("Unknown (");
>+    description.AppendInt((uint32_t)aFindFlag);

Please use static_cast.

>@@ -1582,41 +1724,117 @@ nsTextStore::InsertEmbedded(DWORD dwFlag
>   PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>          ("TSF: 0x%p nsTextStore::InsertEmbedded() called "
>           "but not supported (E_NOTIMPL)", this));
> 
>   // embedded objects are not supported
>   return E_NOTIMPL;
> }
> 
>+void
>+nsTextStore::SetInputScope(const nsString& aHTMLInputType)

>+  } else if (aHTMLInputType.EqualsLiteral("tel")) {
>+    mInputScopes.AppendElement(IS_TELEPHONE_FULLTELEPHONENUMBER);
>+    mInputScopes.AppendElement(IS_TELEPHONE_LOCALNUMBER);

Why do you add IS_TELEPHONE_LOCALNUMBER for this? Does adding this change actual behavior on your environment? If so, why don't you add IS_TELEPHONE_COUNTRYCODE and  IS_TELEPHONE_AREACODE? But I guess that only IS_TELEPHONE_FULLTELEPHONENUMBER is enough like date/time case.

>+HRESULT
>+nsTextStore::ProcessScopeRequest(DWORD dwFlags,
>+                                 ULONG cFilterAttrs,
>+                                 const TS_ATTRID *paFilterAttrs)
>+{
>+  PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>+         ("TSF: 0x%p nsTextStore::ProcessScopeRequest() called "
>+          "cFilterAttrs=%d dwFlags=%s", this, cFilterAttrs,
>+          GetFindFlagName(dwFlags).get()));
>+
>+  // This is a little weird! RequestSupportedAttrs gives us advanced notice
>+  // of a support query via RetrieveRequestedAttrs for a specific attribute.
>+  // RetrieveRequestedAttrs needs to return valid data for all attributes we
>+  // support, but the text service will only want the input scope object
>+  // returned in RetrieveRequestedAttrs if the dwFlags passed in here contains
>+  // TS_ATTR_FIND_WANT_VALUE.
>+  mInputScopeDetected = mInputScopeRequested = false;
>+
>+  // Currently we only support GUID_PROP_INPUTSCOPE
>+  for (uint32_t idx = 0; idx < cFilterAttrs; ++idx) {
>+    PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>+           ("TSF: 0x%p nsTextStore::ProcessScopeRequest() "
>+            "requested attr=%s", this, GetCLSIDNameStr(paFilterAttrs[idx]).get()));

Could you insert 2 spaces after %p? The indent must make the log read easier.

>+    if (IsEqualGUID(paFilterAttrs[idx], GUID_PROP_INPUTSCOPE)) {
>+      PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>+             ("TSF: 0x%p nsTextStore::ProcessScopeRequest() "
>+              "GUID_PROP_INPUTSCOPE queried", this));

Same.

>+      mInputScopeDetected = true;
>+      if (dwFlags & TS_ATTR_FIND_WANT_VALUE) {
>+        PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>+               ("TSF: 0x%p nsTextStore::ProcessScopeRequest() "
>+                "TS_ATTR_FIND_WANT_VALUE specified", this));

Same.

>@@ -1664,19 +1882,46 @@ nsTextStore::RetrieveRequestedAttrs(ULON
>     PR_LOG(sTextStoreLog, PR_LOG_ERROR,
>            ("TSF: 0x%p   nsTextStore::RetrieveRequestedAttrs() FAILED due to "
>             "null argument", this));
>     return E_INVALIDARG;
>   }
> 
>   PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>          ("TSF: 0x%p nsTextStore::RetrieveRequestedAttrs() called "
>-          "but not supported, *pcFetched=0 (S_OK)", this));
>-
>-  // no attributes defined
>+          "ulCount=%d", this, ulCount));
>+
>+  if (mInputScopeDetected || mInputScopeRequested) {
>+    PR_LOG(sTextStoreLog, PR_LOG_ALWAYS,
>+           ("TSF: 0x%p nsTextStore::RetrieveRequestedAttrs() for "

Same above, i.e., please add two spaces after %p.


r=me with the change. Thank you for your work!!!

	Jim Mathies [:jimm] Assignee
	Comment 54 • 12 years ago

> >+void
> >+nsTextStore::SetInputScope(const nsString& aHTMLInputType)
> 
> >+  } else if (aHTMLInputType.EqualsLiteral("tel")) {
> >+    mInputScopes.AppendElement(IS_TELEPHONE_FULLTELEPHONENUMBER);
> >+    mInputScopes.AppendElement(IS_TELEPHONE_LOCALNUMBER);
> 
> Why do you add IS_TELEPHONE_LOCALNUMBER for this? Does adding this change
> actual behavior on your environment? If so, why don't you add
> IS_TELEPHONE_COUNTRYCODE and  IS_TELEPHONE_AREACODE? But I guess that only
> IS_TELEPHONE_FULLTELEPHONENUMBER is enough like date/time case.

For the IS_ values, I just went through the whole list and tried to pick appropriate flags. We can tweak or change these if we find that 3rd party providers use them. On Win8, the soft keyboard brings up the numerics keyboard for these.

	Jim Mathies [:jimm] Assignee
	Comment 56 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/51975f8c58b2

Thanks for all the help!

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 57 • 12 years ago

(In reply to Jim Mathies [:jimm] from comment #54)
> > >+void
> > >+nsTextStore::SetInputScope(const nsString& aHTMLInputType)
> > 
> > >+  } else if (aHTMLInputType.EqualsLiteral("tel")) {
> > >+    mInputScopes.AppendElement(IS_TELEPHONE_FULLTELEPHONENUMBER);
> > >+    mInputScopes.AppendElement(IS_TELEPHONE_LOCALNUMBER);
> > 
> > Why do you add IS_TELEPHONE_LOCALNUMBER for this? Does adding this change
> > actual behavior on your environment? If so, why don't you add
> > IS_TELEPHONE_COUNTRYCODE and  IS_TELEPHONE_AREACODE? But I guess that only
> > IS_TELEPHONE_FULLTELEPHONENUMBER is enough like date/time case.
> 
> For the IS_ values, I just went through the whole list and tried to pick
> appropriate flags. We can tweak or change these if we find that 3rd party
> providers use them. On Win8, the soft keyboard brings up the numerics
> keyboard for these.

I understood that. My idea is, the code should be:

  } else if (aHTMLInputType.EqualsLiteral("tel")) {
    mInputScopes.AppendElement(IS_TELEPHONE_FULLTELEPHONENUMBER);
  } else if (...

Or:

  } else if (aHTMLInputType.EqualsLiteral("tel")) {
    mInputScopes.AppendElement(IS_TELEPHONE_FULLTELEPHONENUMBER);
    mInputScopes.AppendElement(IS_TELEPHONE_COUNTRYCODE);
    mInputScopes.AppendElement(IS_TELEPHONE_AREACODE);
    mInputScopes.AppendElement(IS_TELEPHONE_LOCALNUMBER);
  } else if (...

I don't understand why do you add only IS_TELEPHONE_LOCALNUMBER except IS_TELEPHONE_FULLTELEPHONENUMBER.

My understand is, IS_TELEPHONE_FULLTELEPHONENUMBER includes IS_TELEPHONE_LOCALNUMBER.

I.e., IS_TELEPHONE_FULLTELEPHONENUMBER is:
((IS_TELEPHONE_COUNTRYCODE)?IS_TELEPHONE_AREACODE)?IS_TELEPHONE_LOCALNUMBER

Therefore, I think it's odd you to add only IS_TELEPHONE_LOCALNUMBER without IS_TELEPHONE_COUNTRYCODE and IS_TELEPHONE_AREACODE.

	Ryan VanderMeulen [:RyanVM]
	Comment 58 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/51975f8c58b2