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.

Wonder if this a dupe of bug 876060

Sounds similar actually but I don't see the link background go grey. Can you replicate using this STR?

This actually sounds more a dupe of bug 872961.

(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.

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

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.

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.

@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.

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.

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.

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

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

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.

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

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?

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 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.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 ;)

(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.

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 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 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.

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

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

Comment on attachment 773487 [details] [diff] [review] Alternate patch (probably better) Still early enough in the cycle to take this recent regression fix.