Vasilica Tamas, QA [:vtamas] Reporter
	Description • 9 years ago

Reproducible on: Firefox 45.0a1, Firefox 44.0a2 and Firefox 43 beta 1 under Windows 10

STR
1.Launch Firefox with a clean profile
2.Touch a text box field.

ER
The on-screen keyboard is automatically displayed.

AR
[1] The on-screen keyboard does not appear when a text box is focused.
[2] Exception: The keyboard appears only for a password field when table mode is enabled and e10s is disabled.
*The ui.osk.debug.keyboardDisplayReason: IKPOS: Keyboard presence confirmed.
*The ui.osk.enabled is set to true.


Additional notes:
- This issue is reproducible on Firefox 45.0a1 (2015-11-04), Firefox 44.0a2 (2015-11-04) and Firefox 43 Beta 1 build 2 under Windows 10 64-bit.
- This issue reproduces in both on/off Tablet Mode ways.
- This testing was performed on a Dell XPS 12 which has a hardware keyboard.

	:Gijs (he/him) Assignee
	Comment 1 • 9 years ago

> Can you post a screenshot of using this tool:
> 
> http://www.nirsoft.net/utils/usb_devices_view.html
> 
> on the XPS when the machine is flipped so you can't use the keyboard? Can you show the
> difference between that state and the one where the keyboard *is* usable (ie machine isn't flipped) ?

	Vasilica Tamas, QA [:vtamas] Reporter
	Comment 2 • 9 years ago

Here are the screenshots, but I see no difference between them:

Flipped screen (unusable keyboard): http://i.imgur.com/4qVUgCY.png
Vertical screen (usable keyboard): http://i.imgur.com/0bMB7Hz.jpg

	:Gijs (he/him) Assignee
	Comment 3 • 9 years ago

So both here and in bug 1221961 and for the last comment in bug 1007063, we basically need a way to distinguish the "attached but unusable" keyboard from a USB keyboard.

Chrome doesn't make this distinction and always listens to the device "orientation", so if you use a USB keyboard attached to a tablet, you get a soft keyboard when touching a field. However, it *only* shows the keyboard if a field is focused via touch. We don't currently have that information in our IME handler.

One thing we could do would be whitelisting device IDs for things like the surface pro touch cover and the builtin keyboard on these dell xps machines... but I'm worried that that won't scale very well. I haven't found a generic way to detect these situations - the device view screenshots in this and the surface bug don't show the usb device as disabled or something like that.

Another thing we could do would be trying to ensure we know the source of the ime focus thing so we can get parity with chrome. That'd be a regression in some respects, but better in others.

Jared/Jim, how do you feel about the situation here? Can we think of other ways to tackle the distinction between an external "really-"USB-connected keyboard and the type cover / disabled-convertible-keyboard things?

	Jim Mathies [:jimm]
	Comment 4 • 9 years ago

I think we would like to have both to do this right. I'd bet we can find a way to update IMEStateManager::NotifyIME to pass event source information through, it's available via gecko/dom events. If we don't want to tackle that here, lets get a bug filed on it.

The enabled keyboard detection is probably tricky. The winrt apis manage to do this reliably though so there's probably a combination that works. There are apis for querying a list of devices using the raw input apis - 

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

which might provide connected state. Also there's the WM_INPUT_DEVICE_CHANGE event that fires when devices connect/disconnect. This might pick up on bluetooth/usb devices connecting/disconnecting.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 9 years ago

I spent some time looking at the code around ::CM_Get_Device_ID in WinIMEHandler.cpp today to see if there was a way to determine if the keyboard was connected via USB but I didn't see it. That may get us a small bit of the way there, but I agree that we really need the event source information at the IME level to get this to work correctly.

	:Gijs (he/him) Assignee
	Comment 6 • 9 years ago

(In reply to Jim Mathies [:jimm] from comment #4)
> I think we would like to have both to do this right. I'd bet we can find a
> way to update IMEStateManager::NotifyIME to pass event source information
> through, it's available via gecko/dom events. If we don't want to tackle
> that here, lets get a bug filed on it.

Let's tackle that here. AFAICT these get created here:

https://dxr.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b/dom/events/IMEStateManager.cpp#1311-1327

from here:

https://dxr.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b/dom/events/IMEContentObserver.cpp#1472

from here:

https://dxr.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b/dom/events/IMEContentObserver.cpp#1108

from here:

https://dxr.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b/dom/events/IMEContentObserver.cpp#1141-1149

from here: 

https://dxr.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b/dom/events/IMEContentObserver.cpp#254

which is annoying because at that point we're basically at the Init() and reinitialize calls of the IMEContentObserver, which are still, I imagine, pretty far away from the initial event that triggers the focus here. :-(

Masayuki, can you help? If we need the event that triggered the focus here, where would we get it, and how do we pass this information down without cluttering up all these call paths?

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

What's the "event source information"? Where is it?

	:Gijs (he/him) Assignee
	Comment 8 • 9 years ago

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> What's the "event source information"?

I'd like to know what type of event triggered the IME's focus. Was it a touch, mouse or keyboard event, was it JS/C++/the OS setting focus to the text entry item in question, etc.

> Where is it?

I was hoping you would be able to tell me that. :-)

	:Gijs (he/him) Assignee
	Comment 9 • 9 years ago

Hm, it looks like most calls will be through https://dxr.mozilla.org/mozilla-central/rev/e1ef2be156de1dad31bb4189a51b178b12b23340/editor/libeditor/nsEditorEventListener.cpp#1122-1123 , which is nsEditorEventListener's OnFocus handler. Which doesn't seem to have the source/"cause" of the focus event. :-(

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

How about to cache aAction.mCause in IMEHandler::SetInputContext()? When an editor gets focus, this is called first, then, NOTIFY_IME_OF_FOCUS is sent.

If you need to add other causes to InputContextAction::Cause, you need to hack nsFocusManager and some method callers of that.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 12 • 9 years ago

Not sure if this is a regression from bug 1007063, or just continuing work, but tracking this for now. We could still take a batch in beta.

	:Gijs (he/him) Assignee
	Comment 13 • 9 years ago

Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r?enndeakin,jaws

	:Gijs (he/him) Assignee
	Comment 14 • 9 years ago

Bug 1221947 - part 2: add TOUCH cause to IME Handling and use it to avoid checking for a keyboard, r?masayuki

	:Gijs (he/him) Assignee
	Comment 15 • 9 years ago

So these patches make things wfm.

The first patch adds a flag to nsIFocusManager and uses it in addition to FLAG_BYMOUSE when the mouse event that triggers focus was simulated because of a touch event.

This meant auditing all the uses of FLAG_BYMOUSE, which the patch does. In some cases I've only left comments; if you prefer I can take those out. For this bug I suspect the only change that's actually necessary is the one in the EventStateManager, but I figured we should be consistent. I looked at addons' MXR for people using BYMOUSE, and I don't think anybody will be negatively impacted (like the check in search.xml for which I flagged you, Jared, which I believe is just wrong either way). It is possible some add-ons would work better if they "opted in" to updating, but I've not looked into those cases too deeply.

The second part adds the IME handling portion, which isn't exposed through IDL and therefore shouldn't really break anything else.

What surprised me was that apart from bug 1224233 which threw me a bit, this seems to have worked as a first try, which I found very surprising and makes me think I've missed something. Do let me know if you have ideas about what I did miss. :-)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 16 • 9 years ago

Comment on attachment 8686661 [details]
MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r=jaws,smaug

https://reviewboard.mozilla.org/r/25015/#review22559

r=me for the search.xml changes. Should we add a test for the search suggestions popup on click versus keyboard-initiated focus change?

	:Gijs (he/him) Assignee
	Comment 17 • 9 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Comment on attachment 8686661 [details]
> MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to
> nsIFocusManager, r?enndeakin,jaws
> 
> https://reviewboard.mozilla.org/r/25015/#review22559
> 
> r=me for the search.xml changes. Should we add a test for the search
> suggestions popup on click versus keyboard-initiated focus change?

Let's make one as part of bug 1222971. I don't think the change in this patch warrants creating one.

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

Comment on attachment 8686662 [details]
MozReview Request: Bug 1221947 - part 2: add TOUCH cause to IME Handling and use it to avoid checking for a keyboard, r=masayuki

https://reviewboard.mozilla.org/r/25017/#review22585

::: widget/windows/WinIMEHandler.cpp:42
(Diff revision 1)
> +InputContextAction::Cause IMEHandler::sLastContextActionCause = InputContextAction::CAUSE_UNKNOWN;

nit: Over 80 characters.

::: widget/windows/WinIMEHandler.cpp:353
(Diff revision 1)
> +  sLastContextActionCause = aAction.mCause;

I wonder, if you need to reopen VKB when user tap in editor (i.e., not actually focus changed), you need additional patch. See IMEStateManager::OnClickInEditor(). Although, this might be different bug, though.

::: widget/windows/WinIMEHandler.cpp:700
(Diff revision 1)
> +    Preferences::SetString(kOskDebugReason, L"IKPOS: Used touch to focus control, ignoring keyboard presence");

nit: over 80 characters.

	:Gijs (he/him) Assignee
	Comment 19 • 9 years ago

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #18)
> ::: widget/windows/WinIMEHandler.cpp:353
> (Diff revision 1)
> > +  sLastContextActionCause = aAction.mCause;
> 
> I wonder, if you need to reopen VKB when user tap in editor (i.e., not
> actually focus changed), you need additional patch. See
> IMEStateManager::OnClickInEditor(). Although, this might be different bug,
> though.

I'm not sure I understand. Can you expand on this? It does sound it's unrelated to this bug in that we've always been doing this when focus changed, and not at other times, and so if we want to add stuff for when focus doesn't change that should probably be a different bug?

Did you intend to not give r+, given that besides this you only noted nits? I know mozreview's UI is not always the most intuitive...

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

(In reply to :Gijs Kruitbosch from comment #19)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #18)
> > ::: widget/windows/WinIMEHandler.cpp:353
> > (Diff revision 1)
> > > +  sLastContextActionCause = aAction.mCause;
> > 
> > I wonder, if you need to reopen VKB when user tap in editor (i.e., not
> > actually focus changed), you need additional patch. See
> > IMEStateManager::OnClickInEditor(). Although, this might be different bug,
> > though.
> 
> I'm not sure I understand. Can you expand on this? It does sound it's
> unrelated to this bug in that we've always been doing this when focus
> changed, and not at other times, and so if we want to add stuff for when
> focus doesn't change that should probably be a different bug?
> 
> Did you intend to not give r+, given that besides this you only noted nits?
> I know mozreview's UI is not always the most intuitive...

Not a point of r-. I didn't open issue at here (unchecked the checkbox).

As I said, this should be another bug even if we need to fix so. See bug 507987 (when we developed Firefox for Maemo).

	:Gijs (he/him) Assignee
	Comment 21 • 9 years ago

Comment on attachment 8686662 [details]
MozReview Request: Bug 1221947 - part 2: add TOUCH cause to IME Handling and use it to avoid checking for a keyboard, r=masayuki

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #20)
> Not a point of r-. I didn't open issue at here (unchecked the checkbox).

Ah, yes, but right now the review flag was just cleared, rather than setting r+. Per your comments I'm going to assume you did mean r+. :-)

	:Gijs (he/him) Assignee
	Comment 22 • 9 years ago

Comment on attachment 8686661 [details]
MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r=jaws,smaug

Mike tells me Neil is on PTO, so redirecting the review for this portion to Olli.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 23 • 9 years ago

Comment on attachment 8686661 [details]
MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r=jaws,smaug

Could you test if passing touch flag in FireFocusOnTargetContent would work well. That would be at least consistent.
FireFocusOnTargetContent is called by an event listener dealing with mouse events, so the information is easily available.
But you're probably right with the comment that when dealing with context menu not pretending that focus is from touch is better.
But please test.


For consistency RenderFrameParent::TakeFocusForClick() should use the right flag.
if I read the code correctly, there should be the touch flag always, but
I admit that setup would be a bit fragile given that the method itself doesn't know that it is for touch.
Could you ask kats or botond (https://bugzilla.mozilla.org/show_bug.cgi?id=1129526 added that code.) if it is safe to always pass
the touch flag too?

	:Gijs (he/him) Assignee
	Comment 24 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #23)
> Comment on attachment 8686661 [details]
> MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to
> nsIFocusManager, r?enndeakin,jaws
> 
> Could you test if passing touch flag in FireFocusOnTargetContent would work
> well. That would be at least consistent.
> FireFocusOnTargetContent is called by an event listener dealing with mouse
> events, so the information is easily available.
> But you're probably right with the comment that when dealing with context
> menu not pretending that focus is from touch is better.
> But please test.

I just realized that on Windows, which is where this is being implemented, this is immaterial because:

https://dxr.mozilla.org/mozilla-central/source/dom/xul/nsXULPopupListener.cpp#51

means that this code is all ifdef'd out there. I'll update the flags so the focus info is correct.

> For consistency RenderFrameParent::TakeFocusForClick() should use the right
> flag.
> if I read the code correctly, there should be the touch flag always, but
> I admit that setup would be a bit fragile given that the method itself
> doesn't know that it is for touch.
> Could you ask kats or botond
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1129526 added that code.) if
> it is safe to always pass
> the touch flag too?

Re-reading this code, I realized I didn't look far enough. You're right that this is all touch code anyway. I'll add a third patch and ask for review from kats or botond on that. Thanks!

	:Gijs (he/him) Assignee
	Comment 25 • 9 years ago

Comment on attachment 8686661 [details]
MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r=jaws,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25015/diff/1-2/

	:Gijs (he/him) Assignee
	Comment 26 • 9 years ago

Comment on attachment 8686662 [details]
MozReview Request: Bug 1221947 - part 2: add TOUCH cause to IME Handling and use it to avoid checking for a keyboard, r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25017/diff/1-2/

	:Gijs (he/him) Assignee
	Comment 27 • 9 years ago

Bug 1221947 - part 3: pass touch information to focus manager in RenderFrameParent, r?kats

	:Gijs (he/him) Assignee
	Comment 28 • 9 years ago

Comment on attachment 8686661 [details]
MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r=jaws,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25015/diff/2-3/

	:Gijs (he/him) Assignee
	Comment 29 • 9 years ago

Comment on attachment 8686662 [details]
MozReview Request: Bug 1221947 - part 2: add TOUCH cause to IME Handling and use it to avoid checking for a keyboard, r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25017/diff/2-3/

	:Gijs (he/him) Assignee
	Comment 30 • 9 years ago

Comment on attachment 8687923 [details]
MozReview Request: Bug 1221947 - part 3: pass touch information to focus manager in RenderFrameParent, r?kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25263/diff/1-2/

	:Gijs (he/him) Assignee
	Comment 31 • 9 years ago

Comment on attachment 8686661 [details]
MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r=jaws,smaug

Gahhh, mozreview. :-\

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 32 • 9 years ago

Comment on attachment 8687923 [details]
MozReview Request: Bug 1221947 - part 3: pass touch information to focus manager in RenderFrameParent, r?kats

https://reviewboard.mozilla.org/r/25263/#review22745

	Pulsebot
	Comment 33 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5bc99e6f928
https://hg.mozilla.org/integration/mozilla-inbound/rev/5af628d4e296
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa690942da0d

	Carsten Book [:Tomcat]
	Comment 34 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e5bc99e6f928
https://hg.mozilla.org/mozilla-central/rev/5af628d4e296
https://hg.mozilla.org/mozilla-central/rev/aa690942da0d

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 35 • 9 years ago

Andrei, if we end up uplifting this fix to beta, can your team verify and do a bit of exploratory testing around this feature? We may want to do that for beta 5 or 6.

	:Gijs (he/him) Assignee
	Comment 36 • 9 years ago

Comment on attachment 8686661 [details]
MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r=jaws,smaug

NB: asking for approval for all 3 patches)

Approval Request Comment
[Feature/regressing bug #]: showing the on-screen keyboard on windows 8+
[User impact if declined]: on many Windows touch-screen devices, keyboards are present but can be folded back, in which case we should still show the on-screen keyboard if the user uses touch to interact with the screen. Without this patch, we will not do that, and using Firefox while such machines are in use as tablets would be very hard.
[Describe test coverage new/current, TreeHerder]: There is no test infrastructure for testing with touch machines, so sadly, no.
[Risks and why]: reasonably low considering:
- this feature has had significant QA attention already and this bug and dupes have been the main - almost the only - complaint (the only other one I know of is bug 1211234 which is Microsoft doing us a favour which our pref fails to turn off. We can live with that.).
- we have killswitch prefs both for the feature as a whole and for win8 specifically. On win10 the feature by default will only be usable within tablet mode (there is an OS-level pref to change this).
- I audited the code in mozilla-central as well as in our addons MXR and believe that the patch I landed fixed/addressed all the callsites/uses of the relevant focus manager code. We should update developer documentation when we're sure in which release we're shipping, but I don't really expect fallout - very few places interact with the focus manager so directly. Most code uses events and element.focus() .
[String/UUID change made/needed]: no. I added a constant to the nsIFocusManager.idl, which I believe does not require an IDL uuid rev, so I landed with IGNORE IDL. Uplifts may need to do the same.

	:Gijs (he/him) Assignee
	Comment 37 • 9 years ago

Comment on attachment 8686661 [details]
MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r=jaws,smaug

Approval Request Comment: comment 36

(sorry, I meant to request both at once and forgot)

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 38 • 9 years ago

Gijs, while reviewing the aurora uplift patch, I noticed that patch2 inside it has comments from masayuki on IME related aspects that still show up as "open issues" in MozReview tool. Do these need to be addressed? Or is this patch ready to land on m-a as is?

	:Gijs (he/him) Assignee
	Comment 39 • 9 years ago

(In reply to Ritu Kothari (:ritu) from comment #38)
> Gijs, while reviewing the aurora uplift patch, I noticed that patch2 inside
> it has comments from masayuki on IME related aspects that still show up as
> "open issues" in MozReview tool. Do these need to be addressed? Or is this
> patch ready to land on m-a as is?

The patch is ready to land. mozreview does not update issues addressed after review.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 40 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #39)
> (In reply to Ritu Kothari (:ritu) from comment #38)
> > Gijs, while reviewing the aurora uplift patch, I noticed that patch2 inside
> > it has comments from masayuki on IME related aspects that still show up as
> > "open issues" in MozReview tool. Do these need to be addressed? Or is this
> > patch ready to land on m-a as is?
> 
> The patch is ready to land. mozreview does not update issues addressed after
> review.

Got it. Thanks!

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 41 • 9 years ago

Comment on attachment 8686661 [details]
MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r=jaws,smaug

Improving our experience on windows touch-screen devices with keyboards folded back is a good idea! It seems QE has tested this area a bit and will be doing some focused testing when this is uplifted to Beta. Let's uplift to Aurora44.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 42 • 9 years ago

Comment on attachment 8686662 [details]
MozReview Request: Bug 1221947 - part 2: add TOUCH cause to IME Handling and use it to avoid checking for a keyboard, r=masayuki

Aurora44+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 43 • 9 years ago

Comment on attachment 8687923 [details]
MozReview Request: Bug 1221947 - part 3: pass touch information to focus manager in RenderFrameParent, r?kats

Aurora44+

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 44 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/8e13fe7ab3da
https://hg.mozilla.org/releases/mozilla-aurora/rev/54f8c04cc8d7
https://hg.mozilla.org/releases/mozilla-aurora/rev/36ad43168b8d

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 45 • 9 years ago

Comment on attachment 8686661 [details]
MozReview Request: Bug 1221947 - part 1: add BYTOUCH flag to nsIFocusManager, r=jaws,smaug

Touch screen fix - Approved for uplift to beta - all 3 patches in this bug.

	Andrei Vaida [:avaida]
	Comment 46 • 9 years ago

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #35)
> Andrei, if we end up uplifting this fix to beta, can your team verify and do
> a bit of exploratory testing around this feature? We may want to do that for
> beta 5 or 6.

Sounds good to me, I'll make sure this gets looked at in Beta 5. Leaving the ni? in place as a reminder.

	Vasilica Tamas, QA [:vtamas] Reporter
	Comment 47 • 9 years ago

Performed Exploratory testing on latest Nightly and Aurora under Windows 10 64-bit using a Dell Xps 12 with tablet mode enabled and this is what I have noticed:
- This bug doesn't seem to be fixed on latest Nightly 45.0a1 (2015-11-17), the OSK does not appear when a text box is focused http://i.imgur.com/6fllICZ.png
- The OSK is successfully displayed on latest Aurora 44.0a2 (2015-11-17) while focusing a text box http://i.imgur.com/F0bdh1y.png


Noticed that the value of ui.osk.on_screen_keyboard_path is different on Nightly and Aurora. (compare the attached screenshots)


Also tested under a Dell Inspiron One All-In-One, with no keyboard attached and the OSK was not displayed: http://i.imgur.com/VZsTIw9.jpg.
Should I file a separate bug for Dell Inspiron One All-In-One issue?

	:Gijs (he/him) Assignee
	Comment 48 • 9 years ago

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #47)
> Performed Exploratory testing on latest Nightly and Aurora under Windows 10
> 64-bit using a Dell Xps 12 with tablet mode enabled and this is what I have
> noticed:
> - This bug doesn't seem to be fixed on latest Nightly 45.0a1 (2015-11-17),
> the OSK does not appear when a text box is focused
> http://i.imgur.com/6fllICZ.png
> - The OSK is successfully displayed on latest Aurora 44.0a2 (2015-11-17)
> while focusing a text box http://i.imgur.com/F0bdh1y.png
> Noticed that the value of ui.osk.on_screen_keyboard_path is different on
> Nightly and Aurora. (compare the attached screenshots)

This doesn't make any sense. Both of those debug reason screenshots indicate the keyboard *wasn't* displayed because we found a hardware keyboard. The code in Aurora and Nightly should be the same at this point. However, your build ID is for aurora from yesterday, which by the timestamp in comment #44 wouldn't even have these patches - they landed evening GMT, by which time the build for aurora would have been long finished. You should be using today's build for tests. I'm not even sure yesterday's nightly build had the patches - an 11am GMT nightly merge (comment 34) doesn't usually make that day's nightly.

How are you focusing the textbox? What textbox are you using to test with? (see also the password input bug 1211234 which is relevant here). Note also that, for instance, the search bar has code for clicks close to it to focus it manually - that doesn't propagate any of the focus reasons. We should fix that separately, but for testing this bug you should really just use an in-content HTML page with a plain input field for consistent results.

> Also tested under a Dell Inspiron One All-In-One, with no keyboard attached
> and the OSK was not displayed: http://i.imgur.com/VZsTIw9.jpg.
> Should I file a separate bug for Dell Inspiron One All-In-One issue?

Yes, but that's less relevant here. I'm really more interested in the XPS 12 issue right now.

	:Gijs (he/him) Assignee
	Comment 49 • 9 years ago

I have now verified that the 11-17 nightly indeed didn't have these patches. So please retest with today's nightly and aurora builds.

	Carsten Book [:Tomcat]
	Comment 50 • 9 years ago

part 2 has problems to be uplifted to beta:

grafting 315339:54f8c04cc8d7 "Bug 1221947 - part 2: add TOUCH cause to IME Handling and use it to avoid checking for a keyboard, r=masayuki a=ritu"
merging dom/base/nsFocusManager.cpp
merging widget/IMEData.h
merging widget/windows/WinIMEHandler.cpp
warning: conflicts during merge.
merging widget/windows/WinIMEHandler.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')
merging widget/windows/WinIMEHandler.h
warning: conflicts during merge.
merging widget/windows/WinIMEHandler.h incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

can you take a look, thanks!

	Vasilica Tamas, QA [:vtamas] Reporter
	Comment 51 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #48)

> This doesn't make any sense. Both of those debug reason screenshots indicate
> the keyboard *wasn't* displayed because we found a hardware keyboard. The
> code in Aurora and Nightly should be the same at this point. However, your
> build ID is for aurora from yesterday, which by the timestamp in comment #44
> wouldn't even have these patches - they landed evening GMT, by which time
> the build for aurora would have been long finished. You should be using
> today's build for tests. I'm not even sure yesterday's nightly build had the
> patches - an 11am GMT nightly merge (comment 34) doesn't usually make that
> day's nightly.

Sorry, my mistake, checked again and noticed that was Aurora from 2015-11-18.
Tested today on Nightly 2015-11-18 and the osk is successfully displayed. 


> How are you focusing the textbox? What textbox are you using to test with?
> (see also the password input bug 1211234 which is relevant here). Note also
> that, for instance, the search bar has code for clicks close to it to focus
> it manually - that doesn't propagate any of the focus reasons. We should fix
> that separately, but for testing this bug you should really just use an
> in-content HTML page with a plain input field for consistent results.

I test around the search bars from: location bar, search bar, about:newtab page, about:home page and text boxes from different webpages or http://www.quackit.com/html/codes/html_text_box_code.cfm 


Also encountered 2 new issues: Bug 1226145 and Bug 1226148

	:Gijs (he/him) Assignee
	Comment 52 • 9 years ago

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #51)

Thanks for the quick verification. I'm glad it works now - you had me panicked for a few minutes yesterday! :-)

> Also encountered 2 new issues: Bug 1226145 and Bug 1226148

Thanks. I'll look at these either tomorrow or next week. They're not regressions and should be covered by bug 1224605, I believe.

Did you also file the issue with the Dell Inspiron One All-in-One?

(In reply to Carsten Book [:Tomcat] from comment #50)
> part 2 has problems to be uplifted to beta:
> 
> grafting 315339:54f8c04cc8d7 "Bug 1221947 - part 2: add TOUCH cause to IME
> Handling and use it to avoid checking for a keyboard, r=masayuki a=ritu"
> merging dom/base/nsFocusManager.cpp
> merging widget/IMEData.h
> merging widget/windows/WinIMEHandler.cpp
> warning: conflicts during merge.
> merging widget/windows/WinIMEHandler.cpp incomplete! (edit conflicts, then
> use 'hg resolve --mark')
> merging widget/windows/WinIMEHandler.h
> warning: conflicts during merge.
> merging widget/windows/WinIMEHandler.h incomplete! (edit conflicts, then use
> 'hg resolve --mark')
> abort: unresolved conflicts, can't continue
> (use hg resolve and hg graft --continue)
> 
> can you take a look, thanks!

I'll have a look ASAP - I got sidetracked with bug 1226136 earlier today.

	:Gijs (he/him) Assignee
	Comment 53 • 9 years ago

Landed on beta:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/02acffd9874e
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/73fdfb5d27e7
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/7c2204493788

	Vasilica Tamas, QA [:vtamas] Reporter
	Comment 54 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #52)

> Did you also file the issue with the Dell Inspiron One All-in-One?

Filed Bug 1226522

	Vasilica Tamas, QA [:vtamas] Reporter
	Comment 56 • 9 years ago

Verified fixed on Firefox 45.0a1 (2015-12-02), Firefox 44.0a2 (2015-12-02) and Firefox 43 beta 8 (20151201152349) under Windows 8.1 64-bit desktop mode and Windows 10 64-bit tablet mode on/off.


We also performed a regression testing on Firefox 43 beta 6 under Windows 8.1 and Windows 10 using 2 devices: Dell Xps 12 and Surface Pro 2. Except a few already known bugs, there were some new issues encountered during this testing. 
    - Bugs that are still affecting the feature: https://goo.gl/PHoVHO 
    - Detailed test results: https://goo.gl/t4VPS3


Based on the above mentioned, I am marking this bug as Verified since the other issues are tracked separately.

	Alex Taylor
	Comment 57 • 8 years ago

Apologies in advance if location or format of my query is wrong!

Hi -I've landed here as 43 is now popping the keyboard unexpectedly for me in Windows 10.
My use case where the problem arises is rare but probably not unique:  I am using a Surface Pro 3 without a physical keyboard attached, but in desktop mode with keyboard input via Synergy (http://synergy-project.org/).  In this situation I am using a physical keyboard, but the OSK keeps popping up.
In searching for info I see other users with bluetooth keyboard anomalies (https://support.mozilla.org/questions/1103572).

Is it possible for an option make the OSK behaviour (which I really like in Tablet Mode), be dependent on Windows Tablet Mode?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 58 • 8 years ago

(In reply to Alex Taylor from comment #57)
> Apologies in advance if location or format of my query is wrong!
> 
> Hi -I've landed here as 43 is now popping the keyboard unexpectedly for me
> in Windows 10.
> My use case where the problem arises is rare but probably not unique:  I am
> using a Surface Pro 3 without a physical keyboard attached, but in desktop
> mode with keyboard input via Synergy (http://synergy-project.org/).  In this
> situation I am using a physical keyboard, but the OSK keeps popping up.
> In searching for info I see other users with bluetooth keyboard anomalies
> (https://support.mozilla.org/questions/1103572).
> 
> Is it possible for an option make the OSK behaviour (which I really like in
> Tablet Mode), be dependent on Windows Tablet Mode?

We should only be showing the OSK outside of Tablet mode if you've enabled that behavior in Windows 10. Go to the Windows Settings dialog > Devices > Typing > "Show the touch keyboard or handwriting panel when not in tablet mode and there's no keyboard attached". Set that pref to Off.

	Alex Taylor
	Comment 59 • 8 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #58)
> We should only be showing the OSK outside of Tablet mode if you've enabled
> that behavior in Windows 10. Go to the Windows Settings dialog > Devices >
> Typing > "Show the touch keyboard or handwriting panel when not in tablet
> mode and there's no keyboard attached". Set that pref to Off.
Fantastic - worked perfectly.  I may post the solution over in the support area.
Thanks for the speedy reply, and keep up the great work.