877602 - Deadzone/can't tap link after address bar is scrolled out of view

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Zac C (:zac)]

There appears to a dead zone the size of the address/awesome bar that appears once the address bar is hidden.

Tested on:
HTC Desire HD, Android 3, Nightly 24.0a1 (2013-05-29)
Samsung Galaxy S2, Android 4, Nightly 24.0a1 (2013-05-29)

STR:

1. Open Nightly
2. Go to wiki.mozilla.org
3. Scroll up the page only as many pixels as required to hide the address/awesome bar
4. Tap a <a> link that has newly appeared on the page.

Actual: Link does not work, either short tap or long tap
Expected: The link will go grey, new page/link loads

If you scroll too far then the links beyond will work.

Comment 1

[Aaron Train [:aaronmt]]

Wonder if this a dupe of bug 876060

Comment 2

[Zac C (:zac)]

Sounds similar actually but I don't see the link background go grey.

Can you replicate using this STR?

Comment 3

[:Margaret Leibovic]

This actually sounds more a dupe of bug 872961.

Comment 4

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #3)
> This actually sounds more a dupe of bug 872961.

Although maybe not, since there's no fixed position element at the bottom in this case.

Comment 6

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

I'll take a look at this, as bug 872961 turned out to be a specific instance of this bug in reader mode.

Comment 7

[:Margaret Leibovic]

Nom-ing to track, since bug 872961 was tracking-fennec:23+

Also, bug 872961 was status-firefox22:affected, but this isn't an issue if the dynamic toolbar is disabled, so I don't think we need to worry about it for 22.

Comment 8

[Aaron Train [:aaronmt]]

I can't reproduce this at all on Nightly (06/18). Tried on the Nexus 4 using the steps in comment #0. Each link works fine for me once the toolbar is scrolled off the screen.

Comment 9

[Zac C (:zac)]

Attachment: video of bug replicated

@aaronmt, here is a video of the bug replicated.

This is using 2013-06-18 / 24.0a1

I couldn't use the original STR because the page had changed. For your phone just try and find a page with a link just off the bottom of the page so that it appears once you start scrolling but before the awesome bar has hidden completely.

Comment 10

[Zac C (:zac)]

Forgot to add: in my test case the same link is on screen but also wraps into the deadzone. The normal area can be tapped but the deadzone area of the link cannot be tapped.

Comment 11

[Kevin Brosnan [Ex-Mozilla]]

Google News, http://news.google.com mobile site seems to hit this. Scroll to the bottom of the page and try to click on the classic link.

Comment 12

[Kevin Brosnan [Ex-Mozilla]]

Kats any idea how to go forward on this? We have about 2 more weeks of betas where we accept feature fixes.

Comment 13

[Erin Lancaster [:elan]]

Also need info-ing Cwiiis per his request so this bubbles up to the surface.

Comment 14

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

I dropped the ball on this, sorry. I'm looking at it now. Preliminary investigation shows that the problem is the CSS viewport is not enlarged when the dynamic toolbar is hidden, which causes clicks to that area to get dropped.

Comment 15

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

Narrowing it down some more, the code inside the condition at [1] doesn't run while the page is loading because when the viewport-sizing code is run the page is still loaded and doesn't exceed the viewport bounds. Triggering a viewport size update after the page is loaded (or at least longer) by e.g. doing a rotation fixes this issue. The correct solution here, I think, is to re-run updateViewportSize every time the page size changes because now the viewport size is dependent on the page size. This feels kind of silly though.

[1] https://hg.mozilla.org/mozilla-central/file/dde4dcd6fa46/mobile/android/chrome/content/browser.js#l3774

Comment 16

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

Attachment: Patch

This fixes it, but potentially introduces an infinite-work cycle because changing the CSS viewport could cause the page size to change and so on. Chris, how can I hook this up to the viewport-remeasure throttle to break this infinite cycle?

Comment 17

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

Attachment: Alternate patch (probably better)

So after looking at the code some more I came to the conclusion that the viewport-remeasure code was supposed to handle exactly this case but wasn't actually. I traced it down to what I think is an incorrect if condition. This patch also fixes the bug and I think is a better patch than the previous one.

I has also have some code cleanups in mind for this code which I'll do if you're ok with this patch (shame on me for letting this pass review).

Comment 18

[Chris Lord [:cwiiis]]

Comment on attachment 773487 [details] [diff] [review]
Alternate patch (probably better)

Review of attachment 773487 [details] [diff] [review]:
-----------------------------------------------------------------

Your change looks correct, but then I wonder how I could have made this mistake... Could you craft some test pages and double-check this is ok? I used http://chrislord.net/files/mozilla/test4.html and http://chrislord.net/files/mozilla/fixed7.html and variations thereof when I was testing.

Comment 19

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> I has also have some code cleanups in mind for this code which I'll do if
> you're ok with this patch (shame on me for letting this pass review).

Yes, shame on you ;)

Comment 20

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

(In reply to Chris Lord [:cwiiis] from comment #18)
> Your change looks correct, but then I wonder how I could have made this
> mistake... Could you craft some test pages and double-check this is ok? I
> used http://chrislord.net/files/mozilla/test4.html and
> http://chrislord.net/files/mozilla/fixed7.html and variations thereof when I
> was testing.

These test pages both work fine. I cooked up some variations on those to exercise the code paths and those all also worked as expected.

Comment 21

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

Attachment: Part 2 - cleanup (no functional change)

This is just some cleanup so the code is more readable. I'm a little suspicious of the code that goes "gScreenWidth + gViewportMargins.left + gViewportMargins.right" since everywhere else subtracts the margins. However I decided to leave that as-is in this patch since changing it might actually affect the behaviour on pages in that range and I'm not sure the rounding behaviour will be good in those cases.

Comment 22

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

Comment on attachment 773487 [details] [diff] [review]
Alternate patch (probably better)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dynamic toolbar patches
User impact if declined: If the user scrolls to hide the dynamic toolbar there is a dead zone with the same height as the toolbar at the bottom of the page. The dead zone disappears if the device is rotated or in other scenarios where the CSS viewport is recalculated.
Testing completed (on m-c, etc.): locally so far
Risk to taking this patch (and alternatives if risky): low risk, i think. the code path being modified is only run in a very narrow set of cases, and if the logic is wrong then it shouldn't have much in the way of adverse effects. fennec-only change.
String or IDL/UUID changes made by this patch: none

Comment 23

[Chris Lord [:cwiiis]]

Comment on attachment 773595 [details] [diff] [review]
Part 2 - cleanup (no functional change)

Review of attachment 773595 [details] [diff] [review]:
-----------------------------------------------------------------

This looks a heck of a lot better than my original code (sorry) and indeed does look functionally equivalent, but I'm not 100% certain of that. I trust that you've tested that to be the case in practice (especially, try bing.com and make sure it doesn't oscillate between page sizes).

::: mobile/android/chrome/content/browser.js
@@ +3192,5 @@
> +      if (viewportShouldExcludeHorizontalMargins != this.viewportExcludesHorizontalMargins) {
> +        remeasureNeeded = true;
> +      }
> +    }
> +    if (hasVerticalMargins) {

You could save some here and say if (!remeasureNeeded && hasVerticalMargins), though given we never have horizontal margins, I guess it doesn't really matter.

Comment 24

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

Yup, bing.com seems to work fine. Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4695f7d4ac24
https://hg.mozilla.org/integration/mozilla-inbound/rev/2765ec8ccf3b

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4695f7d4ac24
https://hg.mozilla.org/mozilla-central/rev/2765ec8ccf3b

Comment 26

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/729f7fdd9bdf

Comment 27

[Alex Keybl [:akeybl]]

Comment on attachment 773487 [details] [diff] [review]
Alternate patch (probably better)

Still early enough in the cycle to take this recent regression fix.

Comment 28

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

https://hg.mozilla.org/releases/mozilla-beta/rev/1511d50de368