1301593 - Firefox mispositions touch/hover and displays a black bar when urlbar is visible and Firefox is a smaller multiwindow using Samsung Touchwiz

Status: RESOLVED FIXED

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

Description

[nemo]

Encounterd on my Note 4 in Firefox stable and nightly.
Ensure Multi Window is enabled in preferences, then open firefox in a small window.

This can be done by by swiping down and to the right from the upper right corner or by holding down the back button, then selecting firefox from the app list.

When the URL bar is visible, there is a black bar same height as the url bar at the bottom of a page.  As I scroll down, the url bar starts to disappear and the balck bar shrinks too.

While the url bar is visible, touch/hover interaction with links on the page is offset by a url bar's height.

Fennec has had similar issues recently with touch/hover being incorrectly positioned when the url bar is visible. Those seem to be fixed, but not this.

P.S.  typing this report on the phone to make sure I was writing up the symptoms correctly, and the Firefox insistence on showing a soft keyboard even when I'm using an external bluetooth one is really irritating.  The bluetooth shortcut to hide the soft keyboard that works in every other app has never worked in Firefox.

Comment 1

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

I don't have a device with multiwindow support so I can't debug this directly. Presumably this issue goes away if you disable the "full screen browsing" option from the settings - which will disable the dynamic toolbar.

It sounds the same as bug 1231554. Can you let me know the hardware model number and Android version you are running?

Comment 2

[nemo]

Turning off that setting does indeed fix things (although most of the time I do like the hiding to save space of course).

This is a Galaxy Note 4 SM-N910T running Android 5.0.1

I'm a little skeptical that it could have anything to do with the hardware/android version due to the whole offset position of hover/touch.

It could I guess have something to do with the Touchwiz version.

I'm a bit surprised Firefox Android team doesn't have a ton of Samsung phones lying around.  They aren't exactly rare 

Comment 3

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

(In reply to nemo from comment #2)
> Turning off that setting does indeed fix things (although most of the time I
> do like the hiding to save space of course).
> 
> This is a Galaxy Note 4 SM-N910T running Android 5.0.1

Thanks

> I'm a little skeptical that it could have anything to do with the
> hardware/android version due to the whole offset position of hover/touch.

Based on my investigation in bug 1231554 I'm pretty sure this happens because the Android code in question isn't repositioning the SurfaceView the way it's supposed to. So the entire content area is shifted up by the height of the toolbar, which explains why you see both the empty space at the bottom and why touch events are offset.

> It could I guess have something to do with the Touchwiz version.

It's possible, I'm not really sure what changes they made in TouchWiz.

> I'm a bit surprised Firefox Android team doesn't have a ton of Samsung
> phones lying around.  They aren't exactly rare

Phones have the annoying property of being in one place at one time. Somebody somewhere has a Samsung Note phone but I work from home and can't access it unless I have one myself. Also - as a regular contributor, you should consider yourself part of the "Firefox Android team". :)

Comment 4

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

I pushed a try build to https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d1dd49553db which rips out some of the hacks we needed for Gingerbread devices. Maybe that will make it play better with the Samsung devices. Please try it out once the build is done. If that doesn't work I can't think of anything else - will need to block the device from having "full screen" browsing ability.

Comment 5

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

On IRC nemo said that the try build had the same problem. So I'll just have to block the device from full-screen browsing.

Comment 6

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

Attachment: Bug 1301593 - Block versions of the Galaxy Note 4 from using the dynamic toolbar as well.

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

Comment 7

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> I pushed a try build to
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d1dd49553db which
> rips out some of the hacks we needed for Gingerbread devices.

Note that we can still remove these hacks, so I moved that to bug 1302452.

Comment 8

[nemo]

☹
Is there any chance you can just disable it while in the smaller window mode?  It doesn't seem to happen otherwise.
I guess there's the issue of... Does Samsung even tell you if you're in that mode.

Comment 9

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

I don't know of any API that exposes that information. It looks like Android 7 (Nougat) also has a multiwindow mode and there are APIs to detect that but the samsung version looks to be different.

Comment 10

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

nemo pointed me to http://developer.samsung.com/html/techdoc/ProgrammingGuide_MultiWindow.pdf which does indicate some APIs that we can use. However I'm not sure what the mechanics of accessing those APIs would be - we might have to bundle the JAR into our APK or download it at runtime or whatnot. I don't know if it's worth the trouble.

Comment 11

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

ni to sebestian as well for comment 10, although he's on PTO for another week or so.

Comment 12

[nemo]

I don't know much about Android development, but surely this is just a library that is bundled on the Samsung devices, so that you could do a:
try { Class.forName( "sec.android.app.multiwindow ); }
Without needing to actually bundle or download the jar.

Which, like all things programming, I checked on Stack Overflow to confirm my intuition ☺

http://stackoverflow.com/questions/5794227/how-to-check-if-class-exists-somewhere-in-package

At which point from reading the PDF it seems it would be an onModeChanged event check on isNormalWindow().

Although, I do wonder, from reading the PDF, if changing the flags for the multiwindow creation might help.  I don't know much about this bug, but could maybe disable this "fullscreen app" flag in multiwindow or something?  Maybe that would help work around the bug somehow?


Anyway, I think I'd overall prefer to have broken multiwindow than having this turned off in normal windows on Samsung..

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8790779 [details]
Bug 1301593 - Block versions of the Galaxy Note 4 from using the dynamic toolbar as well.

https://reviewboard.mozilla.org/r/78442/#review79066

Including the library is definitely something we do not want. It would be interesting to know if we could just disable multiwindow mode for Firefox (without the library) though.

Comment 14

[nemo]

Nooooooo.  Please don't disable multiwindow mode on browser!
It's like the most useful app to have multiwindow on!!!

And. I don't see why you have to include the library.  Surely you can just dynamically load it, and turn off the url bar hiding in multiwindow.

Pleeeease.  Man.  This sucks. Might have to setup an android build env to save my browser workflow ☹

Regretting even bring this bug up now.

Would much much much  much much rather have a black bar in multiwindow than no multiwindow at all.

Comment 15

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

The patch disables the dynamic toolbar, not multiwindow. For now you'll still be able to use Firefox in multiwindow, but you won't be able to scroll off the dynamic toolbar. It should be possible to write an add-on to allow fullscreen browsing if you really want it (matt brubeck had one at [1] but it doesn't look like it's been updated in a long time - should be fairly straightforward to do though).

[1] https://addons.mozilla.org/EN-US/mobile/addon/full-screen-252573/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1179307c56b3
Block versions of the Galaxy Note 4 from using the dynamic toolbar as well. r=sebastian

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1179307c56b3

Comment 18

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

Comment on attachment 8790779 [details]
Bug 1301593 - Block versions of the Galaxy Note 4 from using the dynamic toolbar as well.

Approval Request Comment
[Feature/regressing bug #]: new Samsung devices
[User impact if declined]: In multiwindow mode on the Galaxy Note 4, when the dynamic toolbar is hidden, the content is offset visibly and from touch events.
[Describe test coverage new/current, TreeHerder]: no automated tests
[Risks and why]: relatively low risk, we're just disabling the dynamic toolbar on these devices. We did so previously for another set of devices on which the same problem occurred, this just expands the set of devices.
[String/UUID change made/needed]: none

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Nemo, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8790779 [details]
Bug 1301593 - Block versions of the Galaxy Note 4 from using the dynamic toolbar as well.

Seems low risk, fix has stabilized on Nightly for a few days, Aurora51+, Beta50+

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5440c6019b43

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0b08e3d9588e

Comment 23

[nemo]

Since there's a "needinfo" - I don't see any difference in behaviour using latest nightly from https://nightly.mozilla.org

Still have fullscreen triggered on full sized window, still have black bar at the bottom in small window mode while the url bar is visible.

Comment 24

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

Sigh. Could you please run the try build at [1] and save the logcat during startup? It should print out the model and version on your device, so that I can update the code to match that. On my Z3C the output looks like this:

I/DynamicToolbar( 8872): Dynamic toolbar check: version=22, model=Xperia Z3C

Also, to sweeten the deal for you, I took Matt Brubeck's old fullscreen add-on and dusted it off so that it works on modern Fennec. You can find the XPI at [2] (it's unsigned, so you need to set xpinstall.signatures.required to false to install it). Please install it on a nightly build or this try build, disable the dynamic toolbar from the settings, and enter fullscreen using the add-on menu item, and see if it works well in windowed mode. If so I can submit the add-on to AMO so that there will be a signed version available for you to use.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d628f97569d4
[2] https://people.mozilla.org/~kgupta/tmp/fullscreen.xpi

Comment 25

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

Here's a new try push, this one shows the relevant information in an alert dialog on browser startup. Hopefully that makes it easier for you :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=beb0be20c146

Comment 26

[nemo]

Heh. So pretty much the same as from about:support - but I guess more precise right?
version=21, model=SM-N910T

BTW, the last letter of the model for the note 4 series is just to distinguish between models for various countries for purposes of frequencies I think.  There shouldn't be any other difference.

Comment 27

[nemo]

BTW, Google Chrome does fullscreen perfectly, whether windowed or normal.

Comment 28

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

Reopening since per comment 23 the patch didn't fix the issue. Some debugging later I realized it's because of a typo on my part, I used SM-910 as the model number instead of SM-N910 (I missed the "N"). Try push with that fixed in progress at [1] for nemo to test.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4d78a5586ca9692d9bb631619a5ee77da00b9be

Comment 29

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

nemo, can you try the build at https://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-e4d78a5586ca9692d9bb631619a5ee77da00b9be/try-android-api-15/fennec-52.0a1.en-US.android-arm.apk

Comment 30

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

Can't wait forever. Gonna try to get this landed before we ship 50.

Comment 31

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

Attachment: Fix typo

Comment 32

[nemo]

*sigh* Confirmed.  With the new apk, fullscreen is no longer available whether windowed or fullscreen.
(ideally would just be turned off for windowed or work as nicely as chrome, but, yeah, this "fixes" the touch mispositioning and black bar)

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f61b905e3b1e
Fix typo in model number. r=sebastian

Comment 34

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

Comment on attachment 8802579 [details] [diff] [review]
Fix typo

Approval Request Comment
[Feature/regressing bug #]: new Samsung device
[User impact if declined]: In multiwindow mode on the Galaxy Note 4, when the dynamic toolbar is hidden, the content is offset visibly and from touch events.
[Describe test coverage new/current, TreeHerder]: tested by the user who reported the issue (comment 32).
[Risks and why]: Extremely low. The first patch that was supposed to fix this had a typo in the model number, this patch just fixes the typo and has been verified by the reporter using a try build
[String/UUID change made/needed]: none

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8802579 [details] [diff] [review]
Fix typo

Low risk fix, Aurora51+, Beta50+

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f61b905e3b1e

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc4f5a9429a8

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ca14ab5a7f99

Comment 39

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #24)
> Also, to sweeten the deal for you, I took Matt Brubeck's old fullscreen
> add-on and dusted it off so that it works on modern Fennec.

FYI this add-on is now available on AMO: https://addons.mozilla.org/addon/fullscreen-mobile/