1391342 - Toolbar can get stuck partially-visible when focusing a text field in fullscreen mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Test page

Z3C running CyanogenMod 12, latest Fennec nightly

STR:
1. Load attached test page
2. Click on the button to enter fullscreen mode
3. Scroll down to the bottom and put focus in the text field.

This brings up the keyboard but also seems to make the dynamic toolbar partially visible, which it shouldn't do. Hiding the keyboard keeps the toolbar partially visible and generally the experience isn't that great.

Filing this based on the report by MarkHelsinki at https://github.com/staktrace/fennec-fullscreen/issues/1 - it's really a bug in fennec.

Randall, can you take a look when you get a chance?

Comment 1

[Randall Barker [:rbarker]]

I do not this this is dynamic toolbar, looks like Photon bustage to me. What appears to be happening is the real toolbar is showing but the SurfaceView is not resizing for the system bar and the system bar is being rendered on top of the real toolbar which is aligned with the top of the surface view. Maybe UX should take a look?

Comment 2

[Randall Barker [:rbarker]]

Playing with it more, it looks like the system bar is getting into a weird state where the foreground color and back ground color are very close and the system bar is rendering on top of both toolbar and content surface view. Creating a new tab seems to fix the system bar in both color and offset.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Do you know who's working on photon stuff in Fennec? I'm not sure who should look into this.

Comment 4

[Randall Barker [:rbarker]]

:nechen, can you please take a look and see what is causing the system bar to get into a weird state with this test page? Thanks.

Comment 5

[Nevin Chen(Not active on Bugzilla)]

Hi Jing Wei
Could you please check if your patch for bug 1391160 also fix this issue?

Comment 6

[Randall Barker [:rbarker]]

bug 1391160 did not fix the issue. Some one needs to take a look at this.

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1391342 - Keep toolbar hidding while showing keyboard in full screen.

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

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8903090 [details]
Bug 1391342 - Keep toolbar hidding while showing keyboard in full screen.

When we show soft keyboard, we also display toolbar.
Since it's shown for a reason, we should show it currently and also exit Full screen.

Comment 9

[Jing-wei Wu [:jwu]]

Comment on attachment 8903090 [details]
Bug 1391342 - Keep toolbar hidding while showing keyboard in full screen.

https://reviewboard.mozilla.org/r/174864/#review180238

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java:241
(Diff revision 1)
>                      // view for the input method. (Bug 1211848)
>                      v.clearFocus();
>                      v.requestFocus();
>                  }
> +                final Activity context = ActivityUtils.getActivityFromContext(v.getContext());
> +                if (context != null) {

We can call `ActivityUtils#isFullScreen(Activity)` to check if we are in fullscreen or not before actually exiting fullscreen.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8903090 [details]
Bug 1391342 - Keep toolbar hidding while showing keyboard in full screen.

https://reviewboard.mozilla.org/r/174864/#review180944

r+ for the code.

But: This will exit full screen mode whenever the keyboard is shown. Is this really what we want or is this just a workaround? How do other browsers behave? Do we really not want to support use cases where we stay in full screen mode and the user can type text? I honestly don't know. The platform team might have an opinion too. :)

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java:240
(Diff revision 1)
>                      // Marshmallow workaround: The view has focus but it is not the active
>                      // view for the input method. (Bug 1211848)
>                      v.clearFocus();
>                      v.requestFocus();
>                  }
> +                final Activity context = ActivityUtils.getActivityFromContext(v.getContext());

nit: if we call getActivity...() then I expect to get  something we call "activity" instead of "context". :)

Comment 11

[Nevin Chen(Not active on Bugzilla)]

I'm not sure why Chrome won't response to the sample page in comment 0. But I think there's reason for this behavior. 
https://hacks.mozilla.org/2012/01/using-the-fullscreen-api-in-web-browsers/

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8903090 [details]
Bug 1391342 - Keep toolbar hidding while showing keyboard in full screen.

https://reviewboard.mozilla.org/r/174864/#review181434

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java:244
(Diff revision 1)
>                  }
> +                final Activity context = ActivityUtils.getActivityFromContext(v.getContext());
> +                if (context != null) {
> +                    ActivityUtils.setFullScreen(context, false);
> +                }
>                  GeckoAppShell.getLayerView().getDynamicToolbarAnimator().showToolbar(/*immediately*/true);

A different solution is to not call `showToolbar` when we're in fullscreen mode. I think I prefer that better because that matches Chrome behavior.

Comment 13

[Nevin Chen(Not active on Bugzilla)]

Attachment: index.html

The reason why attachment 8898398 [details] not opening in Chrome because it only uese requestFullscreen()
For Chrome to work we need webkitRequestFullscreen()


Testing this example with chrome and Fennec
I found that Chrome will still be in Fullscreen mode and display the softkeyboard (without showing the toobar again.)

I'll follow comment 12. and update the patch... and request review again.

Comment 14

[Nevin Chen(Not active on Bugzilla)]

Current Chrome's implementation has a bug.
It'll show the transparent status bar when the soft keyboard appears.
And in my example here, the input box will be half covered by the status bar.

I don't think we should copy that bug as well.

The best way is to keep status bar hiding and stay full screen when we show soft keyboard.

If this is urgent to land, we can land my current patch and fix it later.
If it's not, I'd like to land after I'm done.

Comment 15

[Nevin Chen(Not active on Bugzilla)]

Hi Jack
Could you please advice what should we do when enter input box while in full screen mode?

Comment 16

[Nevin Chen(Not active on Bugzilla)]

Sample APK for the patch

https://drive.google.com/file/d/0Bw-r8iYPh2ZDQ3JuYlFSNDhuV28/view?usp=sharing

Comment 17

[Wesly Huang (EPM)]

Sorry for late, I think P3 is fair for it so not urgent to land in Nightly 57.

(In reply to Nevin Chen [:nechen] from comment #14)
> If this is urgent to land, we can land my current patch and fix it later.
> If it's not, I'd like to land after I'm done.

Comment 18

[Jack Lin[:jacklin]]

(In reply to Nevin Chen [:nechen] from comment #15)
> Hi Jack
> Could you please advice what should we do when enter input box while in full
> screen mode?

I suggest stay in current mode which users is using.

So as our discussion, showing keyboard but still in full screen mode. And status bar will not cover the page (using transparent status bar like Chrome does).

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Attachment: Test page 2

works with chrome

Comment 20

[Nevin Chen(Not active on Bugzilla)]

Attachment: TestPage2.html

Comment 21

[Nevin Chen(Not active on Bugzilla)]

showSoftInput will be call twice when I click on the input box in the sample page.
The first time is from onCreateInputConnection()[1] , the second time is from restartInput()[2]


[1] is expected. Under fullscreen mode, when the first time showSoftInput() is called, activity's windows flag View.SYSTEM_UI_FLAG_FULLSCREEN is true.
but when the second time,getSystemUiVisibility via View.SYSTEM_UI_FLAG_FULLSCREEN falg will be false. ( at this moment, the keyboard and status bar, navigation bar will apear)

Hi Jim
Can you advise why is that? It's confusing if I want to make  Comment 12 works since the fullscreen state keeps changing.


[1] http://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java#220

[2] http://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java#962

Comment 22

[Nevin Chen(Not active on Bugzilla)]

^^^^ comment 21

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

I think it's enough to add `mIsUserAction = false;` at [1]. That way `showSoftInput()` will only be called once.

[1] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java#654

Comment 24

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8903090 [details]
Bug 1391342 - Keep toolbar hidding while showing keyboard in full screen.

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d2e66b2486d
Keep toolbar hidding while showing keyboard in full screen. r=jchen,jwu,sebastian

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for crashing in testInputConnection:

https://hg.mozilla.org/integration/autoland/rev/451a48bb65e8718e2445138ddbe6f256d5cd98e7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1d2e66b2486d729c58e8f7eb963f9feb964131c6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=136133035&repo=autoland

[task 2017-10-11T06:37:36.314Z] 06:37:36     INFO -  TEST-START | testInputConnection
[task 2017-10-11T06:37:36.315Z] 06:37:36     INFO -  GECKO | EventExpecter: no longer listening for Gecko:Ready
[task 2017-10-11T06:37:36.315Z] 06:37:36     INFO -  GECKO | EventExpecter: no longer listening for Content:DOMContentLoaded
[task 2017-10-11T06:37:36.316Z] 06:37:36     INFO -  GECKO | EventExpecter: no longer listening for Content:DOMTitleChanged
[task 2017-10-11T06:37:57.494Z] 06:37:57     INFO -  INFO | automation.py | Application ran for: 0:01:56.168103
[task 2017-10-11T06:37:57.494Z] 06:37:57     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmp8w2fT5pidlog
[task 2017-10-11T06:37:59.130Z] 06:37:59  WARNING -  PROCESS-CRASH | testInputConnection | java-exception java.lang.ClassCastException: org.mozilla.gecko.tests.components.GeckoViewComponent$TextInput$3 cannot be cast to android.app.Activity at org.mozilla.gecko.GeckoInputConnection.onCreateInputConnection(GeckoInputConnection.java:655)
[task 2017-10-11T06:37:59.130Z] 06:37:59     INFO -  0 ERROR runApp() exited with code 1

Comment 27

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8903090 [details]
Bug 1391342 - Keep toolbar hidding while showing keyboard in full screen.

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c7b05c78551
Keep toolbar hidding while showing keyboard in full screen. r=jchen,jwu,sebastian

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6c7b05c78551

Comment 30

[Nevin Chen(Not active on Bugzilla)]

Thanks!

Comment 31

[Julien Cristau [:jcristau]]

Is this worth uplifting to 57?

Comment 32

[Wesly Huang (EPM)]

I still think P3 is enough so not worth uplifting.

(In reply to Wesly Huang (Fennec Frontend/Firefox EPM) from comment #17)
> Sorry for late, I think P3 is fair for it so not urgent to land in Nightly
> 57.
> 
> (In reply to Nevin Chen [:nechen] from comment #14)
> > If this is urgent to land, we can land my current patch and fix it later.
> > If it's not, I'd like to land after I'm done.

Comment 33

[Nevin Chen(Not active on Bugzilla)]

Thanks Julien
Per comment 32, we don't need to uplift this bug.