856497 - Regression: IMDB search field hidden behind awesomebar

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lawrence Mandel [:lmandel] (use needinfo)]

Attachment: #1 IMDB search bar hidden behind awesomebar

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

Comment 1

[Lawrence Mandel [:lmandel] (use needinfo)]

Attachment: #2 IMDB search bar visible after pulling page down

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

Attachment: #3 IMDB search bar is hidden after giving focus to IMDB search box

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 4

[Aaron Train [:aaronmt]]

Might be the same as bug 854099. Chris?

Comment 5

[Chris Lord [:cwiiis]]

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

Comment 6

[Chris Lord [:cwiiis]]

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

Comment 7

[Chris Lord [:cwiiis]]

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

Comment 8

[Chris Lord [:cwiiis]]

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?

Comment 9

[Ian Barlow (:ibarlow)]

(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

Comment 10

[Chris Lord [:cwiiis]]

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

Comment 11

[Ian Barlow (:ibarlow)]

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

Comment 12

[Chris Lord [:cwiiis]]

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

Comment 13

[Chris Lord [:cwiiis]]

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.

Comment 14

[Chris Lord [:cwiiis]]

Attachment: Fix dynamic viewport resizing

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 16

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

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

Comment 17

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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?

Comment 18

[Chris Lord [:cwiiis]]

Attachment: Fix dynamic viewport resizing v2

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 19

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

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

Comment 20

[Chris Lord [:cwiiis]]

Change made and pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2cc1ec0a50

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a2cc1ec0a50

Comment 22

[Teodora Vermesan (:TeoVermesan)]

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

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 25

[Chris Lord [:cwiiis]]

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

Comment 26

[Chris Lord [:cwiiis]]

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.

Comment 27

[Chris Lord [:cwiiis]]

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.

Comment 28

[Chris Lord [:cwiiis]]

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.

Comment 29

[Chris Lord [:cwiiis]]

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

Comment 30

[Aaron Train [:aaronmt]]

Fixed via bug 858969

Comment 31

[Wesley Johnston (:wesj)]

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 32

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

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 33

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 34

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

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 35

[Alex Keybl [:akeybl]]

Comment on attachment 734646 [details] [diff] [review]
Fix dynamic viewport resizing v2

Approving to prevent the regression in bug 856497

Comment 36

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e2e93d5247f3

Comment 37

[Paul Feher]

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