1239744 - Don't auto-show the on-screen keyboard for automatic focus gains that aren't the result of a touch/tap

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[:Gijs (he/him)]

It seems at least Chrome doesn't do this, and the behaviour is part of what is annoying for users right now. If you open a new tab, or open a website that focuses an input box (e.g. yahoo.com), you don't necessarily want to type, and the keyboard that pops up prevents you from interacting with the lower half of the page. If you do want to type, tapping in the box is only 1 extra interaction.

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1239744 - no longer automatically show an on-screen keyboard for programmatic focus changes, tidy up osk logic in IMEHandler, r?masayuki

Review commit: https://reviewboard.mozilla.org/r/33811/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33811/

Comment 2

[:Gijs (he/him)]

Jared, requesting review from you as well because this seems an 'interesting' change... it'd be cool if you could take it for a spin and see if there's anything we do 'wrong' now. I didn't find anything in a bit of cursory testing, but 6 eyes see more than 4... :-)

Comment 3

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

Comment on attachment 8716395 [details]
MozReview Request: Bug 1239744 - no longer automatically show an on-screen keyboard for programmatic focus changes, tidy up osk logic in IMEHandler, r?masayuki

https://reviewboard.mozilla.org/r/33811/#review30509

::: widget/windows/WinIMEHandler.cpp:768
(Diff revision 1)
> +      sLastContextActionCause == InputContextAction::CAUSE_UNKNOWN_CHROME) {

Why do you need to refuse CAUSE_UNKNOWN_CHROME too? Our Android widget does like this: http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=75dfe10ec44a#2780

Comment 4

[:Gijs (he/him)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> Comment on attachment 8716395 [details]
> MozReview Request: Bug 1239744 - ignore focus changes with no listed cause,
> r?jaws,masayuki
> 
> https://reviewboard.mozilla.org/r/33811/#review30509
> 
> ::: widget/windows/WinIMEHandler.cpp:768
> (Diff revision 1)
> > +      sLastContextActionCause == InputContextAction::CAUSE_UNKNOWN_CHROME) {
> 
> Why do you need to refuse CAUSE_UNKNOWN_CHROME too? Our Android widget does
> like this:
> http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.
> cpp?rev=75dfe10ec44a#2780

When you open Firefox (by default opening about:home and focusing the search bar in there) and/or open a new tab, those focus switches seem to use CAUSE_UNKNOWN_CHROME. I don't think we should show an OSK in that case.

Comment 5

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

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> > Comment on attachment 8716395 [details]
> > MozReview Request: Bug 1239744 - ignore focus changes with no listed cause,
> > r?jaws,masayuki
> > 
> > https://reviewboard.mozilla.org/r/33811/#review30509
> > 
> > ::: widget/windows/WinIMEHandler.cpp:768
> > (Diff revision 1)
> > > +      sLastContextActionCause == InputContextAction::CAUSE_UNKNOWN_CHROME) {
> > 
> > Why do you need to refuse CAUSE_UNKNOWN_CHROME too? Our Android widget does
> > like this:
> > http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.
> > cpp?rev=75dfe10ec44a#2780
> 
> When you open Firefox (by default opening about:home and focusing the search
> bar in there) and/or open a new tab, those focus switches seem to use
> CAUSE_UNKNOWN_CHROME. I don't think we should show an OSK in that case.

Thank you for the actual scenario. Indeed, I agree with that.

But I don't like your approach here because your change may be broken by other developers when they change InputContextAction::Cause (especially, when they add new causes).

I guess that you want to stop opening OSK when it's caused by *non-user-input*.

If so, you should create a static method in IMEData.h, e.g.,

in InputContextAction,

static bool IsUserAction(InputContextAction::Cause aCause)
{
  switch (aCause) {
    case CAUSE_KEY:
    case CAUSE_MOUSE:
    case CAUSE_TOUCH:
      return true;
    default:
      return false;
  }
}

Anyway, you should create a similar method into InputContextAction for making your patch safer.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8716395 [details]
MozReview Request: Bug 1239744 - no longer automatically show an on-screen keyboard for programmatic focus changes, tidy up osk logic in IMEHandler, r?masayuki

https://reviewboard.mozilla.org/r/33811/#review30659

I can't really test this patch out locally, because I need to disable the keyboard detection to show the on-screen keyboard for my machine. With keyboard detection enabled, I get "IKPOS: Rotation sensor not found".

I do think we may have gotten to the point where IsKeyboardPresentOnSlate does a little more than the name of the function alludes to. Now not only is it checking if a keyboard is present, but it's also looking at other parts of the environment and state, such as checking if the last event was a touch event.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8716395 [details]
MozReview Request: Bug 1239744 - no longer automatically show an on-screen keyboard for programmatic focus changes, tidy up osk logic in IMEHandler, r?masayuki

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

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8716395 [details]
> MozReview Request: Bug 1239744 - no longer automatically show an on-screen
> keyboard for programmatic focus changes, tidy up osk logic in IMEHandler,
> r?masayuki
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/33811/diff/1-2/

Note that based on comment 6 I also split out the keyboard detection to its own function, and wrote a separate 'parent' function that contains all the other 'should we show an on-screen keyboard' logic. I thought making a method called "ShouldNotShowOnScreenKeyboard" would be dumb (no negations in method names!) so I named it "NeedOnScreenKeyboard()" and inversed all the boolean returns in it. That's harder to review, and I'm sorry about that, but hopefully it makes the code cleaner.

Comment 9

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

Comment on attachment 8716395 [details]
MozReview Request: Bug 1239744 - no longer automatically show an on-screen keyboard for programmatic focus changes, tidy up osk logic in IMEHandler, r?masayuki

https://reviewboard.mozilla.org/r/33811/#review31003

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3bb18e558f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e3bb18e558f

Comment 12

[:Gijs (he/him)]

Comment on attachment 8716395 [details]
MozReview Request: Bug 1239744 - no longer automatically show an on-screen keyboard for programmatic focus changes, tidy up osk logic in IMEHandler, r?masayuki

Approval Request Comment
[Feature/regressing bug #]: on-screen keyboard support
[User impact if declined]: keyboard pops up too often on win8+ which is annoying for users
[Describe test coverage new/current, TreeHerder]: nope :-(
[Risks and why]: reasonably low, as this is a relatively small change (I know, the patch looks a little bit bigger, but the actual logical change is pretty small). Reducing the impact of the on-screen keyboard when people don't want it
[String/UUID change made/needed]: nope

Comment 13

[Liz Henry (:lizzard)]

Comment on attachment 8716395 [details]
MozReview Request: Bug 1239744 - no longer automatically show an on-screen keyboard for programmatic focus changes, tidy up osk logic in IMEHandler, r?masayuki

Sounds useful, may be good for touch screen users
Please uplift to aurora.

Comment 14

[Liz Henry (:lizzard)]

Florin this might be worth a little exploratory testing in beta 46 from your team, or maybe just bringing this to the attention of whoever might be testing on-screen keyboards in Windows.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f69ff5ea8b0d

Comment 16

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Florin this might be worth a little exploratory testing in beta 46 from your
> team, or maybe just bringing this to the attention of whoever might be
> testing on-screen keyboards in Windows.

I think you are right. Ica and Cornel tested this prior to release in 43, but someone else may also do the testing depending on personal workload.

Assigning to Andrei so this gets in the team's backlog.

Comment 17

[Andrei Vaida [:avaida]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #16)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> > Florin this might be worth a little exploratory testing in beta 46 from your
> > team, or maybe just bringing this to the attention of whoever might be
> > testing on-screen keyboards in Windows.
> 
> I think you are right. Ica and Cornel tested this prior to release in 43,
> but someone else may also do the testing depending on personal workload.
> 
> Assigning to Andrei so this gets in the team's backlog.

I'll make sure this gets looked at during Beta 46. I'm keeping the ni? for now, as a reminder.

Comment 18

[Ada [:adalucinet]]

Some exploratory testing was performed with 46.0b4 build 1 and latest Aurora 47.0a2 (from 2016-03-22), under Windows 10 64-bit on Dell Xps 12 and Surface Pro 2 devices. One new issue was found - bug 1259063 - and since this is tracked separately, marking here accordingly.

Comment 19

[Will]

I think there is either a bug or an issue with this behavior though: on a Windows tablet, when you click the Find in page magnifying glass (Ctrl+F) the on-screen keyboard SHOULD come up, you shouldn't have to also tap in the Find box again in order to bring up the keyboard. in Edge on the Surface Pro 4, when you tap Find on page, the on-screen keyboard does indeed come up. Should this be added to #1224605?

Comment 20

[Will]

Also, I think when a Windows tablet user opens a new tab, the on-screen keyboard should probably come up automatically. I'm sure there are other examples where the on-screen keyboard should come up automatically

Comment 21

[:Gijs (he/him)]

(In reply to Will from comment #19)
> Should this be added to #1224605?

Thanks for bringing this up. No, there should be a separate bug filed for these cases. I filed bug 1263369 and CC'd you.