When loading m.imdb.com, the search field is hidden behind the awesomebar after the page loads (screenshot #1). I can pull the page down to reveal the IMDB search bar (screenshot #2). However, when I give focus to the search box, the IMDB search bar once again moves under the awesomebar (screenshot #3). This is a new issue since awesomebar hiding was enabled (bug 716403).

Tested on Nightly 22.0a1 2013-03-31 on Samsung Galaxy S2

Might be the same as bug 854099. Chris?

(In reply to Aaron Train [:aaronmt] from comment #4) > Might be the same as bug 854099. Chris? I don't think this is quite the same. Seems our 'scroll into view' code probably needs to take into account the fixed viewport margins - I'm not sure if we do this in browser.js or rely on something in Gecko, but it may well be a different code-path that also needs fixing.

With a patch applied for bug 854099, this isn't fixed... Will have a look.

Ok, so the page just scrolls to (0,2) on load. I don't know why it does that, possibly to get rid of the header on things like iPhones or Android stock browser? I can confirm that the Android browser has the same behaviour, except they hide their toolbar after load (like the initial version of the dynamic toolbar patch used to do ;)) so it isn't as obvious. I don't like the idea of hiding the toolbar if the page scrolls as this seems like an easy way to trick users into pressing things they didn't intend - I can think of no other ways to fix this other than to hide the toolbar on page scroll (unacceptable due to the prior reason), or to hide on load. Anything else I can think of would either mess with DOM state too much or expose too many details to content... Or just be too hacky... Not sure what we want to do with this. Needinfo'ing ibarlow for UX comment (re: hide-on-load as a possible solution).

Perhaps as a semi-solution, we could hide the toolbar after load if the page has scrolled down and the user hasn't scrolled at all? If the page has scrolled down, hiding the toolbar won't move the page, so there isn't that to worry about. Ian?

(In reply to Chris Lord [:cwiiis] from comment #8) > Perhaps as a semi-solution, we could hide the toolbar after load if the page > has scrolled down and the user hasn't scrolled at all? If the page has > scrolled down, hiding the toolbar won't move the page, so there isn't that > to worry about. Ian? Can we know that? That sounds like a good solution. I'm also wondering if we really need to re-show the URL bar when you focus an input field in a website. Seems unnecessary, and breaks this page as shown in comment 2

(In reply to Ian Barlow (:ibarlow) from comment #9) > (In reply to Chris Lord [:cwiiis] from comment #8) > > Perhaps as a semi-solution, we could hide the toolbar after load if the page > > has scrolled down and the user hasn't scrolled at all? If the page has > > scrolled down, hiding the toolbar won't move the page, so there isn't that > > to worry about. Ian? > > Can we know that? That sounds like a good solution. We can indeed, will do this. Thanks :) > I'm also wondering if we really need to re-show the URL bar when you focus > an input field in a website. Seems unnecessary, and breaks this page as > shown in comment 2 We don't re-show the URL bar when you focus an input field, if that's happening it's a bug - we only re-show at the moment when page loading starts.

(In reply to Chris Lord [:cwiiis] from comment #10) > > We don't re-show the URL bar when you focus an input field, if that's > happening it's a bug - we only re-show at the moment when page loading > starts. Oops you're right. I tried it again and misunderstood what had happened. Focusing the field *while the title bar is open* actually pushes the field under the title bar when the keyboard comes up.

(In reply to Ian Barlow (:ibarlow) from comment #11) > (In reply to Chris Lord [:cwiiis] from comment #10) > Oops you're right. I tried it again and misunderstood what had happened. > Focusing the field *while the title bar is open* actually pushes the field > under the title bar when the keyboard comes up. There's a bug open for this... But it's not blocking dynamic-toolbar and I appear to have lost it :/ Maybe Aaron knows?

I'm wrong about this. I have a patch that fixes it - why I was getting it scrolling to 0,2, I'm not sure, maybe that was caused by a bounce animation getting triggered or something? We won't need to do what I suggested in comment #8, patch incoming.

This fixes: - Page size rounding errors stopping viewport changes in browser.js - The viewport being remeasured on viewport updates that weren't page size updates in browser.js - The viewport not being remeasured when fixed margins changed in browser.js - The possibility of the margin size message getting lost on startup in BrowserApp.java - Margins not being correctly squashed when the page fits inside the viewport in GeckoLayerClient.java, or squashing being incorrectly compensated This likely fixes some issues on duckduckgo.com mentioned in bug 858181, and *may* fix bug 855019. This ought to fix quite a lot of wonkiness we may have been seeing.

Comment on attachment 734019 [details] [diff] [review] Fix dynamic viewport resizing Review of attachment 734019 [details] [diff] [review]: ----------------------------------------------------------------- While it looks technically correct I'd really like to see the GLC code reshaped for readability. If you're in a hurry to get this in I wouldn't mind deferring that to another bug though. ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +280,5 @@ > + - (metrics.pageRectLeft - metrics.viewportRectLeft)); > + adjustedMargins.right = > + Math.min(adjustedMargins.right + (metrics.fixedLayerMarginLeft - adjustedMargins.left), > + maxMarginWidth - adjustedMargins.left); > + changed = true; This min/max code is getting pretty confusing to follow. Can we instead rewrite this block like so: float maxMarginWidth = Math.max(0, metrics.getPageWidth() - metrics.getWidthWithoutMargins()); float leftOverscroll = metrics.pageRectLeft - metrics.viewportRectLeft; if (leftOverscroll > 0 && metrics.fixedLayerMarginLeft > 0) { if (adjustedMargins.left > leftOverscroll) { adjustedMargins.left -= leftOverscroll; adjustedMargins.right += leftOverscroll; } else { adjustedMargins.left = 0; adjustedMargins.right = maxMarginWidth; } } I'm not sure this is exactly equivalent, but my general thought is that your code can be re-shaped using if conditions to be much clearer (and specifically, to better map to the comment block above) than with min/max calls everywhere. I might be fine with replacing that inner else block with a | leftOverscroll = Math.min(leftOverscroll, adjustedMargins.left) | before the inner if, but I still think that sacrifices readability a bit. (Ditto for the rest of the edges with appropriate modifications.)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16) > Comment on attachment 734019 [details] [diff] [review] > Fix dynamic viewport resizing > > Review of attachment 734019 [details] [diff] [review]: > ----------------------------------------------------------------- > > While it looks technically correct I'd really like to see the GLC code > reshaped for readability. If you're in a hurry to get this in I wouldn't > mind deferring that to another bug though. > > ::: mobile/android/base/gfx/GeckoLayerClient.java > @@ +280,5 @@ > > + - (metrics.pageRectLeft - metrics.viewportRectLeft)); > > + adjustedMargins.right = > > + Math.min(adjustedMargins.right + (metrics.fixedLayerMarginLeft - adjustedMargins.left), > > + maxMarginWidth - adjustedMargins.left); > > + changed = true; > > This min/max code is getting pretty confusing to follow. Can we instead > rewrite this block like so: > > float maxMarginWidth = Math.max(0, metrics.getPageWidth() - > metrics.getWidthWithoutMargins()); > float leftOverscroll = metrics.pageRectLeft - metrics.viewportRectLeft; > if (leftOverscroll > 0 && metrics.fixedLayerMarginLeft > 0) { > if (adjustedMargins.left > leftOverscroll) { > adjustedMargins.left -= leftOverscroll; > adjustedMargins.right += leftOverscroll; > } else { > adjustedMargins.left = 0; > adjustedMargins.right = maxMarginWidth; > } > } > > I'm not sure this is exactly equivalent, but my general thought is that your > code can be re-shaped using if conditions to be much clearer (and > specifically, to better map to the comment block above) than with min/max > calls everywhere. I might be fine with replacing that inner else block with > a | leftOverscroll = Math.min(leftOverscroll, adjustedMargins.left) | before > the inner if, but I still think that sacrifices readability a bit. > > (Ditto for the rest of the edges with appropriate modifications.) An earlier version of the patch did split this into > 0 and else cases, but I found that I was getting rounding errors, to the point that after adding all the code to deal with that, it felt like it'd be much sturdier to do it in a way that would always apply correctly, regardless of fractional scroll values (and after some testing, I would say that this was a good assumption). I would prefer to leave it like that tbh. On the other hand, having a separate variable to store overscroll and make the code more readable is a good idea and I'll do that. This would be much more readable if Java had a Math.clamp... Maybe we should add this to one of our Utils classes at some point?

This does mostly the same thing, but is *much* simplified (thanks to a weekend's rest :)). Resubmitting as it's significantly different in adjustedFixedMarginsForOverscroll.

Comment on attachment 734646 [details] [diff] [review] Fix dynamic viewport resizing v2 Review of attachment 734646 [details] [diff] [review]: ----------------------------------------------------------------- Much better, thanks! ::: mobile/android/base/util/FloatUtils.java @@ +31,5 @@ > + * Returns 'value', clamped so that it isn't any lower than 'low', and it > + * isn't any higher than 'high'. > + */ > + public static float clamp(float value, float low, float high) { > + return Math.max(low, Math.min(high, value)); Throw an exception if low > high

Change made and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2cc1ec0a50

Verified fixed on: -build: Firefox for Android 23.0a1 (2013-04-09) -device: Samsung Galaxy SII -OS: Android 4.0.3

I can still reproduce this issue on Firefox for Android Nightly 23.0a1 2013-04-15. When I load m.imdb.com the search field is hidden by the awesomebar. When I pull the page down and select the search field it jumps back up to being hidden by the awesomebar. If I pull the page back down I can type in the search field and the page remains in the same position. When I click enter to execute a search, the page then jumps back to hiding the search field under the awesomebar.

Seems it's regressed? Possibly down to site changes, looking at it.

So comment #7 is happening again, but perhaps the page scroll is as a result of viewport resizing, or an aborted animation? I'll investigate further.

ok, so I've stepped through this, and looked at the page source - it does indeed just scroll downwards on load. You can load m.idmb.com watch the same thing happen on desktop Firefox. We can either ignore this, do as I say in comment #7, or do as I say here: https://bugzilla.mozilla.org/show_bug.cgi?id=858969#c3 The latter is the hardest, but likely most reliable solution, comment #7 isn't entirely trivial but is easier, and of course, ignoring it is the easiest :) Starting to think the latter solution might be worth doing now, but it's pretty involved... I'll revisit this tomorrow.

Having discussed it a bit and slept on it, I think, despite the effort, it's worth fixing bug 858969 - It ought to, overall, simplify the code and provide more reliable behaviour.

It doesn't really serve us to have this bug open anymore. I'm duping this to 858969.

Fixed via bug 858969

Comment on attachment 734646 [details] [diff] [review] Fix dynamic viewport resizing v2 Noming this to fix bug 852986. kats, you know this code much better than I, so also asking you for feedback. Can you give a risk assessment (you suggested this in bug 852986)? [Approval Request Comment] Bug caused by (feature/regressing bug #): Dynamic toolbar User impact if declined: Pressing back can lead to pages zoomed in. Testing completed (on m-c, etc.): This has been on mc for awhile. I've tested on beta and it does fix the problem. Risk to taking this patch (and alternatives if risky): I'm not able to make a good assessment here. The code seems sane. Kats, can you? String or IDL/UUID changes made by this patch: None.

Comment on attachment 734646 [details] [diff] [review] Fix dynamic viewport resizing v2 I would put this at medium risk, but if it regresses things it will only be a small set of dynamic toolbar cases and shouldn't have a huge impact. I'm in favour of uplifting this. Since wesj said he tested this fix, I assume it applied without too much trouble, which is a good sign.

Comment on attachment 734646 [details] [diff] [review] Fix dynamic viewport resizing v2 This looks different than the fix landed in bug 858969, which this was duped to, so please re-open this bug if this still needs to land.

Changing the status here to fixed instead of duplicate, since the patch on this bug did land in m-c. The duplicate status appears to be causing much confusion.

Comment on attachment 734646 [details] [diff] [review] Fix dynamic viewport resizing v2 Approving to prevent the regression in bug 856497

Verified fixed on: -build: Firefox for Android 23.0b1 (2013-06-26) -device: LG Optimus 4x -OS: Android 4.1.2