If one visits Google Trends on Fennec and taps the search/magnifying glass icon, the page's background dims but the IME keyboard does not pop up. Based on mozregression this began happening when bug 1400878 landed, suggesting that the fix there is not quite web-compatible.

Snorp, should we track this for 57?

(In reply to Jim Mathies [:jimm] from comment #1) > Snorp, should we track this for 57? We should, it's a regression from 56 that we need to fix.

We think this might be fixed, can you retest?

Snorp, should we back out bug 1400878 to fix in 57?

Bug 1400878 makes sure there's a user action. I wonder if Google is somehow intercepting the focus/click/whatever event, and dispatching an untrusted event after the fact? Would that affect us opening the keyboard?

(In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from comment #5) > Bug 1400878 makes sure there's a user action. I wonder if Google is somehow > intercepting the focus/click/whatever event, and dispatching an untrusted > event after the fact? Would that affect us opening the keyboard? At least calling focus() directly from an onclick handler in a parent/sibling element seems to work fine. But I don't know what Google is actually doing there... (In reply to Jim Mathies [:jimm] from comment #3) > We think this might be fixed, can you retest? Still broken in today's Nightly.

I'm afraid it still isn't working on the latest nightly. I still see the Trends search interface come up, but the keyboard does not. Mike seems to be right; I see mousedown, "$md.pressdown", click, and focus events being dispatched via EventTarget.prototype.dispatchEvent, triggered by this Angular element: ><button class="md-icon-button md-no-ink search-button search-button-mobile md-button md-ink-ripple" type="button" ng-transclude="" aria-label="Search" ng-click="showMobileSearchDialog()"> It goes through typical AngularJS event-handling goop, with no special handlers: >o https://ajax.googleapis.com/ajax/libs/angular_material/1.1.0-rc4/angular-material.min.js:7:20500 >onStart https://ajax.googleapis.com/ajax/libs/angular_material/1.1.0-rc4/angular-material.min.js:7:17672 >start https://ajax.googleapis.com/ajax/libs/angular_material/1.1.0-rc4/angular-material.min.js:7:20890 >i https://ajax.googleapis.com/ajax/libs/angular_material/1.1.0-rc4/angular-material.min.js:7:21849 >a https://ajax.googleapis.com/ajax/libs/angular_material/1.1.0-rc4/angular-material.min.js:7:21937 >Je https://ssl.gstatic.com/trends_nrtr/1173_RC01/third_parties_min.js:4667:186 >c https://ssl.gstatic.com/trends_nrtr/1173_RC01/third_parties_min.js:4667:135 (an in-file copy of AngularJS v1.5.12) Ultimately, it calls the expected method: >Iq.prototype.showMobileSearchDialog_ = function() { > Hq.show({ > clickOutsideToClose: !0, > controller: "SearchDialogCtrl", > controllerAs: "ctrl", > templateUrl: "/components/search/search-dialog.html", > onComplete: function(a, b) { > b.find("input").focus() > } > }) >}; But the focus() call fails (though it *is* run). But since there doesn't seem to be anything special going on, just usual AngularJS stuff, I worry that this means other Angular apps will be affected as well.

(In reply to Jim Mathies [:jimm] from comment #4) > Snorp, should we back out bug 1400878 to fix in 57? I don't want to do that if we can avoid it. I'm looking at a fix for this right now.

The easiest fix here would be to allow some period of time past the last user action where we still show the keyboard. The problem is this will likely break the original fix where e.g. you click a link, navigate to a new page which calls .focus() on an element there. Also Speedometer. The user activity tracking is global for every document and I think we need it to be per content PresShell or something along those lines. Masayuki, what do you think? For 57 it might be best to just back out the original patch, but we might be able to get the per-PresShell thing into 58 still.

Yeah, per-PresShell sounds reasonable. But how about focus moving across iframe boundary? I guess, should it be per-root-PresShell? If you think that we should land the patch for bug 1406446 first, I'll do that even before Gijs is back.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10) > Yeah, per-PresShell sounds reasonable. But how about focus moving across > iframe boundary? I guess, should it be per-root-PresShell? I was looking at this a bit last night and I think maybe we already have enough information. Can we use nsFocusManager::GetFocusedWindow() to stop us from allowing vkb on a page that the user did not focus themselves? > > If you think that we should land the patch for bug 1406446 first, I'll do > that even before Gijs is back. I think that patch does make some things clearer, so I'm ok with that.

[Tracking Requested - why for this release]: Keyboard input on Google Trends is broken

> Bug 1400878 makes sure there's a user action. So this is intended behavior in Firefox for Android now? The keyboard won't show up, unless there is a user-invoked event on top of the stack? If that's the case, then it's the same as on Apple devices, although it's strange, since the behavior differs from Chrome on Android. But there is a difference between Safari's behavior and FF for Android: Safari on iOS doesn't allow setting focus to the input fields programmatically at all, which in its turn prevents the keyboard popup. But in the current FF 57.0 you technically move the focus, but prevent a keyboard from showing, which is strange. How can I see if it worked or not (..and require an additional action from the user) ? P.S. I'll duplicate the links I attached to Bug 1418481 : A simple jsfiddle example to test the issue: https://jsfiddle.net/cu7z307s/5/ Recorded video of the issue in FF and Chrome: https://www.youtube.com/watch?v=3oY6ZIzzMAo

(In reply to Oleksandr Zhurbenko from comment #15) > > Bug 1400878 makes sure there's a user action. > > So this is intended behavior in Firefox for Android now? > The keyboard won't show up, unless there is a user-invoked event on top of > the stack? > Yeah. I think we need to change it use a timeout instead of requiring it to be inside the click handler. This is likely what Chrome is doing.

Actually Chrome doesn't seem to use a timeout. I think maybe it only blocks focus during page load or something. We should probably just back the original change out.

OK it looks like Chrome blocks IME until there has been at least one user interaction on the page[0]. This seems reasonable. The only problem for us is we don't track this per-page, which brings us back to comment #10. [0] https://groups.google.com/a/chromium.org/forum/#!topic/blink-reviews/0e8woFAUIpM

Comment on attachment 8930713 [details] Bug 1409113 - Add nsIPresShell::HasHandledUserInput() https://reviewboard.mozilla.org/r/201832/#review207210 ::: layout/base/PresShell.cpp:8005 (Diff revision 1) > sLastInputProcessed = now; > } > > + if (aEvent->AsInputEvent()) { > + mHasHandledUserInput = true; > + } > + > return rv; > } Why don't you make mHasHandledUserInput to true before dispatching an event? This method is used by some other event dispatchers too. E.g., PointerEvent is dispatched via this method. So, if you set this to true here, the state while dispatching first input event is really unstable, i.e, it may be set to true while *dispatching* first event. So, I think that setting this to true at very first of this method is reasonable. Additionally, perhaps, checking only |aEvent->AsInputEvent()| isn't enough to check if the event is a user input. I think that you should create a method, |bool IsUserAction() const|, in WidgetEvent and implement it in WidgetEventImpl.cpp with the following body: bool WidgetEvent::IsUserAction() const { if (!IsTrusted()) { return false; } // FYI: eMouseScrollEventClass and ePointerEventClass represent // user action but they are synthesized events. switch (mClass) { case eKeyboardEventClass: case eCompositionEventClass: case eMouseScrollEventClass: case eWheelEventClass: case eGestureNotifyEventClass: case eSimpleGestureEventClass: case eTouchEventClass: case eCommandEventClass: case eContentCommandEventClass: case ePluginEventClass: return true; case eMouseEventClass: case eDragEventClass: case ePointerEventClass: return AsMouseEvent()->IsReal(); default: return false; } } ::: layout/base/nsIPresShell.h:1654 (Diff revision 1) > virtual void NotifyStyleSheetServiceSheetRemoved(mozilla::StyleSheet* aSheet, > uint32_t aSheetType) = 0; > - > protected: I don't think removing empty line here is reasonable change of this bug.

Comment on attachment 8930714 [details] Bug 1409113 - Add nsIPresShell::HasHandledUserInput() status to InputContext https://reviewboard.mozilla.org/r/201834/#review207232 ::: dom/events/IMEStateManager.cpp:1282 (Diff revision 1) > context.mIMEState = aState; > context.mOrigin = aOrigin; > context.mMayBeIMEUnaware = context.mIMEState.IsEditable() && > sCheckForIMEUnawareWebApps && MayBeIMEUnawareWebApp(aContent); > > + context.mHasHandledUserInput = aPresContext && aPresContext->PresShell()->HasHandledUserInput(); nit: Too long line. Please break after |=|.

Comment on attachment 8930715 [details] Bug 1409113 - Relax the user input requirement in order to show VKB on Android https://reviewboard.mozilla.org/r/201836/#review207238 ::: widget/android/GeckoEditableSupport.cpp:1382 (Diff revision 1) > return; > } > mIMEUpdatingContext = true; > > RefPtr<GeckoEditableSupport> self(this); > - bool isUserAction = aAction.IsHandlingUserInput(); > + bool isUserAction = aAction.IsHandlingUserInput() || aContext.mHasHandledUserInput; Perhaps, we should change WinIMEHandler too: https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/widget/windows/WinIMEHandler.cpp#449,462 And the patch should be reviewed by Gijs.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #11) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10) > > Yeah, per-PresShell sounds reasonable. But how about focus moving across > > iframe boundary? I guess, should it be per-root-PresShell? > > I was looking at this a bit last night and I think maybe we already have > enough information. Can we use nsFocusManager::GetFocusedWindow() to stop us > from allowing vkb on a page that the user did not focus themselves? Oh, sorry, I missed to catch this question. I think that it's not necessary because IMEStateManager sets widget to new input context only for the active window's widget. And looks like that the approach of your patches are right.

Comment on attachment 8930713 [details] Bug 1409113 - Add nsIPresShell::HasHandledUserInput() https://reviewboard.mozilla.org/r/201832/#review207434

Comment on attachment 8930715 [details] Bug 1409113 - Relax the user input requirement in order to show VKB on Android https://reviewboard.mozilla.org/r/201836/#review207442

Pushed by jwillcox@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e1ece860e4e Add nsIPresShell::HasHandledUserInput() r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/4503ee61c1b3 Add nsIPresShell::HasHandledUserInput() status to InputContext r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b10d0a0465 Relax the user input requirement in order to show VKB on Android r=jchen

Comment on attachment 8930713 [details] Bug 1409113 - Add nsIPresShell::HasHandledUserInput() Approval Request Comment [Feature/Bug causing the regression]: Bug 1400878 [User impact if declined]: Unable to type into input fields in certain cases [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Not yet [Needs manual test from QE? If yes, steps to reproduce]: Yes. Ensure you can use the search field on trends.google.com. [List of other uplifts needed for the feature/fix]: Just the three patches in this bug [Is the change risky?]: No [Why is the change risky/not risky?]: Only changes a minor detail of how the vkb is shown [String changes made/needed]: None

https://hg.mozilla.org/mozilla-central/rev/2e1ece860e4e https://hg.mozilla.org/mozilla-central/rev/4503ee61c1b3 https://hg.mozilla.org/mozilla-central/rev/e7b10d0a0465

Tested in the Nightly for Android, it's working fine. Thanks for the quick fix guys! Now I only need to have workarounds for FF 57 and 58.

As we already shipped 56 with this bug and this didn't cause too much noise, I don't think it is critical to take that in a 57 dot release.

Comment on attachment 8930713 [details] Bug 1409113 - Add nsIPresShell::HasHandledUserInput() But taking it in 58.

https://hg.mozilla.org/releases/mozilla-beta/rev/60ef3560d15a https://hg.mozilla.org/releases/mozilla-beta/rev/c97b0b34d8cc https://hg.mozilla.org/releases/mozilla-beta/rev/d0eadbf8ab5b

Verified as fixed on Beta 58.0b7. Devices: LG Nexus 5 (Android 6.0.1) Google Pixel (Android 8.0)