Closed Bug 1224605 Opened 4 years ago Closed 4 years ago

Investigate opening the on-screen keyboard in more cases

Categories

(Core :: Widget: Win32, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(2 files)

From bug 1221947:


(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #20)
> (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?

> As I said, this should be another bug even if we need to fix so. See bug
> 507987 (when we developed Firefox for Maemo).
Masayuki-san, I tried to investigate this a bit, but I'm struggling. There's a few issues:

1) we try to keep track of whether the osk is open, but AFAICT we don't account for users closing the OSK themselves.
2) IMEContentObserver::OnMouseButtonEvent bails out if there is no text under the mouse cursor when clicking into the input box
3) That same mnethod does not pass input source information and so we don't know if clicks are from the touch screen or not.

All of these are fixable, I think, but I'm just wondering if I'm missing something more obvious. Can you take a look?
Flags: needinfo?(masayuki)
Attachment #8696677 - Flags: feedback?(masayuki)
Comment on attachment 8696677 [details] [diff] [review]
Strawman patch to toggle the OSK when touching already-focused inputs

>-  InputContextAction action(InputContextAction::CAUSE_MOUSE,
>-                            InputContextAction::FOCUS_NOT_CHANGED);
>+  uint16_t inputSource = nsIDOMMouseEvent::MOZ_SOURCE_UNKNOWN;
>+  aMouseEvent->GetMozInputSource(&inputSource);
>+  InputContextAction::Cause cause =
>+    inputSource == nsIDOMMouseEvent::MOZ_SOURCE_TOUCH ?
>+    InputContextAction::CAUSE_TOUCH : InputContextAction::CAUSE_MOUSE;

nit: please insert 2 spaces to this line.

>diff --git a/widget/windows/WinIMEHandler.cpp b/widget/windows/WinIMEHandler.cpp
>--- a/widget/windows/WinIMEHandler.cpp
>+++ b/widget/windows/WinIMEHandler.cpp
>@@ -231,16 +231,17 @@ IMEHandler::NotifyIME(nsWindow* aWindow,
>       }
>       case NOTIFY_IME_OF_BLUR:
>         sFocusedWindow = nullptr;
>         IMEHandler::MaybeDismissOnScreenKeyboard();
>         IMMHandler::OnFocusChange(false, aWindow);
>         return TSFTextStore::OnFocusChange(false, aWindow,
>                                            aWindow->GetInputContext());
>       case NOTIFY_IME_OF_MOUSE_BUTTON_EVENT:
>+        IMEHandler::MaybeShowOnScreenKeyboard();

Oops, no.

> 2) IMEContentObserver::OnMouseButtonEvent bails out if there is no text under the mouse cursor when clicking into the input box

OnMouseButtonEvent() is designed for making native IME on desktop platforms handle click in composition string. So, NOTIFY_IME_OF_MOUSE_BUTTON_EVENT is not related to this bug.

You fix right point in IMEStateManager, though. IMEStateManager::OnClickInEditor() causes an additional call of nsIWidget::SetInputContext() which calls IMEHandler::SetInputContext() on Windows. So, you need to call MaybeShowOnScreenKeyboard() when aAction.UserMightRequestOpenVKB() of SetInputContext() returns true (the method should be modified too). The method is defined in IMEData.h.
Flags: needinfo?(masayuki)
Attachment #8696677 - Flags: feedback?(masayuki) → feedback-
Bug 1224605 - also show the OSK when tapping in focused inputs, r?masayuki
Attachment #8697134 - Flags: review?(masayuki)
Comment on attachment 8697134 [details]
MozReview Request: Bug 1224605 - also show the OSK when tapping in focused inputs, r?masayuki

https://reviewboard.mozilla.org/r/27521/#review24799

::: widget/windows/WinIMEHandler.cpp:369
(Diff revision 1)
> +  if (aAction.UserMightRequestOpenVKB()) {
> +    IMEHandler::MaybeShowOnScreenKeyboard();
> +  }

I think that this doesn't cause any problems actually. But it might be safer you do this after sPluginHasFocus is modified. Even without this change, this patch looks good to me.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/bfa8b0451cfc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1226145
You need to log in before you can comment on or make changes to this bug.