Closed Bug 1391342 Opened 2 years ago Closed 2 years ago

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

Categories

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

57 Branch
All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: kats, Assigned: cnevinchen)

Details

(Whiteboard: [SP=X, 8][FNC][SPT58.2][INT])

Attachments

(3 files, 2 obsolete files)

Attached file 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?
Flags: needinfo?(rbarker)
Component: General → Graphics, Panning and Zooming
Assignee: nobody → rbarker
Flags: needinfo?(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?
Assignee: rbarker → nobody
Status: ASSIGNED → NEW
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.
Do you know who's working on photon stuff in Fennec? I'm not sure who should look into this.
No longer blocks: dynamic-toolbar-3
: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.
Flags: needinfo?(cnevinchen)
Hi Jing Wei
Could you please check if your patch for bug 1391160 also fix this issue?
Flags: needinfo?(cnevinchen) → needinfo?(topwu.tw)
bug 1391160 did not fix the issue. Some one needs to take a look at this.
Flags: needinfo?(cnevinchen)
Assignee: nobody → cnevinchen
Flags: needinfo?(cnevinchen)
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 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.
Attachment #8903090 - Flags: review?(topwu.tw) → review+
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". :)
Attachment #8903090 - Flags: review?(s.kaspari) → review+
Attachment #8903090 - Flags: review?(nchen)
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 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.
Attachment #8903090 - Flags: review?(nchen) → review+
Attached file index.html (obsolete) —
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.
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.
Flags: needinfo?(wehuang)
Hi Jack
Could you please advice what should we do when enter input box while in full screen mode?
Flags: needinfo?(jalin)
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.
Flags: needinfo?(wehuang)
(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).
Flags: needinfo?(jalin)
Attached file Test page 2 (obsolete) —
works with chrome
Attached file TestPage2.html
Attachment #8905796 - Attachment is obsolete: true
Attachment #8911029 - Attachment is obsolete: true
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 21
Flags: needinfo?(nchen)
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
Flags: needinfo?(nchen)
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d2e66b2486d
Keep toolbar hidding while showing keyboard in full screen. r=jchen,jwu,sebastian
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
Flags: needinfo?(cnevinchen)
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c7b05c78551
Keep toolbar hidding while showing keyboard in full screen. r=jchen,jwu,sebastian
https://hg.mozilla.org/mozilla-central/rev/6c7b05c78551
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Thanks!
Flags: needinfo?(cnevinchen)
Whiteboard: [SP=X, 8]
Is this worth uplifting to 57?
Flags: needinfo?(cnevinchen)
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.
Thanks Julien
Per comment 32, we don't need to uplift this bug.
Flags: needinfo?(cnevinchen)
Whiteboard: [SP=X, 8] → [SP=X, 8][FNC][SPT58.2][INT]
You need to log in before you can comment on or make changes to this bug.