Closed Bug 1301593 Opened 3 years ago Closed 3 years ago

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

Categories

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

48 Branch
ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: bugs, Assigned: kats)

References

Details

Attachments

(2 files)

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.
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?
Flags: needinfo?(bugs)
Version: Trunk → 48 Branch
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 
Flags: needinfo?(bugs)
(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". :)
Assignee: nobody → bugmail
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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
☹
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.
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.
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.
ni to sebestian as well for comment 10, although he's on PTO for another week or so.
Flags: needinfo?(s.kaspari)
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 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.
Attachment #8790779 - Flags: review?(s.kaspari) → review+
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.
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/
Flags: needinfo?(s.kaspari)
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1179307c56b3
Block versions of the Galaxy Note 4 from using the dynamic toolbar as well. r=sebastian
https://hg.mozilla.org/mozilla-central/rev/1179307c56b3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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
Attachment #8790779 - Flags: approval-mozilla-beta?
Attachment #8790779 - Flags: approval-mozilla-aurora?
Hello Nemo, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(bugs)
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+
Attachment #8790779 - Flags: approval-mozilla-beta?
Attachment #8790779 - Flags: approval-mozilla-beta+
Attachment #8790779 - Flags: approval-mozilla-aurora?
Attachment #8790779 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(bugs)
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
Flags: needinfo?(bugs)
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
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.
Flags: needinfo?(bugs)
BTW, Google Chrome does fullscreen perfectly, whether windowed or normal.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can't wait forever. Gonna try to get this landed before we ship 50.
*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)
Flags: needinfo?(bugs)
Attachment #8802579 - Flags: review?(s.kaspari) → review+
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
Attachment #8802579 - Flags: approval-mozilla-beta?
Attachment #8802579 - Flags: approval-mozilla-aurora?
Comment on attachment 8802579 [details] [diff] [review]
Fix typo

Low risk fix, Aurora51+, Beta50+
Attachment #8802579 - Flags: approval-mozilla-beta?
Attachment #8802579 - Flags: approval-mozilla-beta+
Attachment #8802579 - Flags: approval-mozilla-aurora?
Attachment #8802579 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/f61b905e3b1e
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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/
You need to log in before you can comment on or make changes to this bug.