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

RESOLVED FIXED in Firefox 58

Status

()

Firefox for Android
Toolbar
P3
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: kats, Assigned: nechen)

Tracking

57 Branch
Firefox 58
All
Android
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

Created attachment 8898398 [details]
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)
(Reporter)

Updated

4 months ago
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: 1335895
: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)
(Assignee)

Comment 5

4 months ago
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)

Updated

4 months ago
Assignee: nobody → cnevinchen
Flags: needinfo?(cnevinchen)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

4 months ago
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

4 months ago
mozreview-review
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 10

4 months ago
mozreview-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+
(Assignee)

Updated

4 months ago
Attachment #8903090 - Flags: review?(nchen)
(Assignee)

Comment 11

4 months ago
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

3 months ago
mozreview-review
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+
(Assignee)

Comment 13

3 months ago
Created attachment 8905796 [details]
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.
(Assignee)

Comment 14

3 months ago
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)
(Assignee)

Comment 15

3 months ago
Hi Jack
Could you please advice what should we do when enter input box while in full screen mode?
Flags: needinfo?(jalin)
(Assignee)

Comment 16

3 months ago
Sample APK for the patch

https://drive.google.com/file/d/0Bw-r8iYPh2ZDQ3JuYlFSNDhuV28/view?usp=sharing
Flags: needinfo?(topwu.tw)
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)

Comment 18

3 months ago
(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)
(Assignee)

Comment 19

3 months ago
Created attachment 8911029 [details]
Test page 2

works with chrome
(Assignee)

Comment 20

3 months ago
Created attachment 8911030 [details]
TestPage2.html
Attachment #8905796 - Attachment is obsolete: true
Attachment #8911029 - Attachment is obsolete: true
(Assignee)

Comment 21

3 months ago
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
(Assignee)

Comment 22

3 months ago
^^^^ 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)
Comment hidden (mozreview-request)

Comment 25

2 months ago
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)
Comment hidden (mozreview-request)

Comment 28

2 months ago
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
Last Resolved: 2 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(Assignee)

Comment 30

2 months ago
Thanks!
Flags: needinfo?(cnevinchen)
(Assignee)

Updated

2 months ago
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.
(Assignee)

Comment 33

2 months ago
Thanks Julien
Per comment 32, we don't need to uplift this bug.
Flags: needinfo?(cnevinchen)
status-firefox57: affected → wontfix
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.