search

mozilla-mobile / fenix

⚠️ Fenix (Firefox for Android) moved to a new repository. It is now developed and maintained as part of: https://github.com/mozilla-mobile/firefox-android
https://github.com/mozilla-mobile/firefox-android
Mozilla Public License 2.0
6.47k stars 1.27k forks source link

[Bug] Dynamic Nav Bar leaves white space on certain websites #8768

Closed serovar closed 4 years ago

serovar commented 4 years ago

Steps to reproduce

Go to your feeds on feedbin.com and scroll down

Expected behavior

The bottom nav bar hides itself leaving way for the webpage content.

Actual behavior

The bottom nav bar hides itself but leaves a white space.

Device information

screen

┆Issue is synchronized with this Jira Task

TitanNano commented 4 years ago

This also happens on:

poperigby commented 4 years ago

Also having this issue on https://www.gog.com/forum

KWierso commented 4 years ago

I have Twitter running in Firefox Preview Nightly as a PWA, and it acts weird, too. image

I don't know if it's this exact issue, but it started happening after Nightly updated this morning.

ekager commented 4 years ago

Starbucks order page behavior with dynamic bottom toolbar (this is a PWA-able page but not using it as a PWA) ezgif com-video-to-gif

TitanNano commented 4 years ago

@sblatz your title change is not correct. This affects both PWA standalone activity and the browser activity. For many sites the bottom and top toolbar leave a white gap when they disappear. If the same page is opened in PWA mode, the white gap is always present.

browser_mode normal browsing

pwa_mode_small PWA mode

sblatz commented 4 years ago

@snorp mentioned it's related to this GV issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1618582

serovar commented 4 years ago

Edited title since I noticed that the problem remains even with the nav bar on top.

snorp commented 4 years ago

The GV changes have landed, so now we'll be able to use this in Fenix. A while back we added a separate onTouchEventForResult() method to the GeckoView class. Instead of returning a boolean as onTouchEvent() does, it returns an enum (well, int) indicating how the event was consumed[1]. There are three values: UNHANDLED, HANDLED, and HANDLED_CONTENT. Fenix should only move the toolbar if it gets a HANDLED result back from this. That means that we used it to do a panning or zooming operation (and also passed it on to web content). If Fenix gets UNHANDLED, it should make the toolbar visible and not move it in response to further events for that touch. This is what will happen for non-scrollable pages. HANDLED_CONTENT means event was in a region where the page has touch event listeners (e.g., Google Maps). Fenix could probably treat this the same as UNHANDLED because it should not move the toolbar in such a case. Whether it should also make the toolbar visible is debatable, but probably the safest thing.

[1] https://mozilla.github.io/geckoview/javadoc/mozilla-central/org/mozilla/geckoview/PanZoomController.html#INPUT_RESULT_HANDLED

liuche commented 4 years ago

Also see #9031 - we can check this for when we pick up these GV changes in AC.

If @sblatz has other things he's working on next week (let me know!), we should be able to find someone to take a look at this. Nav Bar is Nightly only right now, so this won't be hitting any release users yet.

sblatz commented 4 years ago

Based on snorp's comment this will require an AC patch. I'm looking into it now.

sblatz commented 4 years ago

I have poked around on this on the AC side. What's odd is that I'm able to fix it in AC but not in Fenix. We must be doing something else on the Fenix side. I'm going to keep investigating here.

sblatz commented 4 years ago

Submitted an AC patch: https://github.com/mozilla-mobile/android-components/pull/6244

bogas04 commented 4 years ago

Possible duplicate https://github.com/mozilla-mobile/fenix/issues/8777

abodea commented 4 years ago

Might be related https://github.com/mozilla-mobile/fenix/issues/9226.

petterill commented 4 years ago

Possible duplicates #9371 and #9299

hiikezoe commented 4 years ago

Note that issues with top toolbar transition are #6940.

abodea commented 4 years ago

I can still reproduce issue following the examples from above, is the fix already in latest nightly @sblatz? Re-checked this on the latest Nightly 3/26 with Google Pixel 4 XL (Android 10). I will remove the eng:qa:needed until further notice,

20200326-145939 20200326-145826

TitanNano commented 4 years ago

this also still affects many PWAs

hiikezoe commented 4 years ago

The issue on https://squoosh.app is a different one. I should note that if the content is scrollable, it's not the same issue. The issue on squoosh looks https://bugzilla.mozilla.org/show_bug.cgi?id=1619169, but I am not 100% sure.

On starbucks it looks correct to me.

Anyways, on Pixel4, as far as I know, WebRender is enabled by default so there is another issue remaining (https://bugzilla.mozilla.org/show_bug.cgi?id=1610731).

hiikezoe commented 4 years ago

The issue on squoosh.app also happens with the top toolbar, it makes me wonder the site does something when scrolling happens, if it does it's an issue in the site itself I think.

karlcow commented 4 years ago

fwiw it creates a certain number of webcompat issues reports.

TitanNano commented 4 years ago

@hiikezoe with out any investigation, I'd say the page is setting a container to 100% height and then scrolls inside the container.

hiikezoe commented 4 years ago

That's super helpful info. So, I think I was confused by the vertical scroll bar there. I thought the scrollbar is for the root scroll container, but it seems not. That means that the dynamic toolbar shouldn't be moved at all.

@snorp, @sblatz, I believe the issue on squoosh.app is still an issue in interactions with GV and Fenix.

Rorschach1010 commented 4 years ago

I only use 2 PWAs but both have this issue (Instagram and Twitter). Waiting patiently for a fix.

sblatz commented 4 years ago

Snorp seems to have fixed this with the bugzilla ticket: https://bugzilla.mozilla.org/show_bug.cgi?id=1627737

fixed scrolling 2020-04-13 11_26_58

sblatz commented 4 years ago

I'm filing a follow up for PWA's (like Twitter): https://github.com/mozilla-mobile/fenix/issues/9892

lobontiumira commented 4 years ago

This issue is not fixed yet. Tested on the following sites: feedbin.com, squoosh.app, maps.google.com, gog.com/forum, twitter.com, starbucks.com, cdc.gov, instagram.com, krcw.com, bnonews.com, and wp.pl, with OnePlus 5T (Android 9), Google Pixel (Android 10), and Sony Xperia Z5 Premium (Android 7.1.1), with the latest Nightly build from 4/16.

On Google Pixel the following pages, the dynamic nav bar leaves spaces:

sblatz commented 4 years ago

@softvision-miralobontiu that looks like the correct behavior to me. If all you're seeing is the small gap that happens while scrolling that's the best we can do for now. If the gap persists and doesn't follow the scroll that's what we're worried about.

Did all of the sites you tested have the behavior shown in your GIF? If so I think we can close this!

serovar commented 4 years ago

I think you can't close this yet.

feedbin

feedbin.com, Nightly 200416 18:00 (Build #21071807) (the latest).

sblatz commented 4 years ago

@serovar we are having delays with our nightly builds right now. This should be fixed on master.

serovar commented 4 years ago

I know, my Nightly is the latest one from here, you can check the build that I wrote.

hiikezoe commented 4 years ago

There seems a rare condition that the touch handling results are misrecognized on images in the case of feedbin?

serovar commented 4 years ago

It does it only with two rapid downward scrolls.

hiikezoe commented 4 years ago

It might be better to open a new issue to track down the race.

serovar commented 4 years ago

It does it with a single swipe if the bar is on top, are you sure that I should open a separate issue? And, if yes, should I open two separate issues for the top bar and the the bottom one?

Top bar feedbintop

Bottom bar feedbindown

hiikezoe commented 4 years ago

The top bar case is totally different one. https://github.com/mozilla-mobile/fenix/issues/6940. sblatz has been working hard on the issue.

Blaze-Thunder commented 4 years ago

This bug is not solved yet. I am on latest Fenix Nightly 200420 06:01. On Telegram WebApp, there is still white bar present at bottom.

hiikezoe commented 4 years ago

As far as I can tell, the PWA case should have been fixed in https://github.com/mozilla-mobile/fenix/issues/9892

@Blaze-Thunder it's worth filing a new issue with detailed descriptions. Thanks!

lobontiumira commented 4 years ago

@softvision-miralobontiu that looks like the correct behavior to me. If all you're seeing is the small gap that happens while scrolling that's the best we can do for now. If the gap persists and doesn't follow the scroll that's what we're worried about.

Did all of the sites you tested have the behavior shown in your GIF? If so I think we can close this!

@sblatz I could see the small gap only on Google Pixel, on the following pages

I saw the exact behavior like in the GIF on all 4 pages from above. I'll remove the qa:needed label for now.

sblatz commented 4 years ago

This also affects Telegram as mentioned here

karlcow commented 4 years ago

for those checking only their email notifications, there seems to be a big class of issues related to this or close that are reported through webcompat. You can see the links here in this issue going to the webpage.

sblatz commented 4 years ago

I'm not able to reproduce this on the latest nightly on:

maps.google.com (PWA or regular site) telegram.com (PWA or regular site) twitter.com (PWA or regular site)

QA can you please verify?

hiikezoe commented 4 years ago

Note that if you still see blanks at the top and the bottom of the contents, please check whether WebRender is enabled or not in about:support. If WebRender is enabled, it's a known issue, https://bugzilla.mozilla.org/show_bug.cgi?id=1619169 I've started looking into.

lobontiumira commented 4 years ago

Verified as fixed on the latest Nightly build from 6/5 with Samsung Galaxy Note 8 (Android 9), Sony Xperia Z5 Premium (Android 7.1.1), and OnePlus 5T (Android 9), on the following pages:

serovar commented 4 years ago 
* feedbin.com

I don't know if it makes sense to consider feedbin.com as "fixed", since now if you login (using it as in the screencast in the first post, not simply going to feedbin.com without logging in) the navigation bar is static (i.e., it does not hide itself when scrolling).

lobontiumira commented 4 years ago

Hi @serovar, The issue was regarding the dynamic bar leaving white spaces on certain websites. I've retested on feedbin.com with the latest Nightly build from 6/11 with Samsung Galaxy Tab S6 (Android 9), signed in with a valid account, and no such white spaces where displayed. Indeed, the bottom navigation bar is fixed, is not dynamic - this should be a new issue.

Setting the navigation bar to the top doesn't display the white space either, but the navigation bar set at the top is dynamic.

serovar commented 4 years ago

Setting the navigation bar to the top doesn't display the white space either

With the navbar to the top, I still get the white space at the bottom on the latest nightly (Nightly 200611 06:01, Build #2015745587). Should I open a new issue for this too?

feedbin top navbar

lobontiumira commented 4 years ago

I will reopen the issue, I was able to reproduce the white spaces having the navigation bar set to the top, on 6/11 Nightly build using Google Pixel (Android 10), Google Pixel 3a (Android 10), and Sony Xperia Z5 Premium (Android 7.1.1). I've tested on a tablet before, and it didn't reproduce there. Thank you!

sv-ohorvath commented 4 years ago

There is https://github.com/mozilla-mobile/fenix/issues/11389 covering the same issue, so we can close this.

I will reopen the issue, I was able to reproduce the white spaces having the navigation bar set to the top, on 6/11 Nightly build using Google Pixel (Android 10), Google Pixel 3a (Android 10), and Sony Xperia Z5 Premium (Android 7.1.1). I've tested on a tablet before, and it didn't reproduce there. Thank you!

imannms commented 3 years ago

After 1 year and this bug still exists