852158 - Pages sometimes shift under the toolbar while loading

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Chris Lord [:cwiiis]]

Sometimes while loading, or when the scroll position changes back to the top, it ends up covering the toolbar. There's code specifically to avoid this, which obviously isn't working. Will take a look.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Fix content scrolling to the top of the page with the toolbar visible

This provides the behaviour I originally intended and makes interacting with the Twitter mobile site *much* nicer.

Comment 2

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

Comment on attachment 726214 [details] [diff] [review]
Fix content scrolling to the top of the page with the toolbar visible

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

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +348,5 @@
> +            // accordingly.
> +            if (type == ViewportMessageType.UPDATE &&
> +                FloatUtils.fuzzyEquals(newMetrics.viewportRectTop,
> +                                       newMetrics.pageRectTop) &&
> +                oldMetrics.fixedLayerMarginTop > 0) {

style nit: When formatting multi-line conditions like this I find it easier to read if the '&&' is at the start of the next line rather than at the end of the previous line. That way looking down the left margin the additional conditions don't look like statements inside the if-body.

i.e.
if (type == ViewportMessageType.UPDATE
    && FloatUtils.fuzzyEquals(...)
    && ...) {
    ...
}

Even better would be an extra level of indentation to differentiate it more from the body:

if (type == ViewportMessageType.UPDATE
        && FloatUtils.fuzzyEquals(...)
        && ...) {
    ...
}

@@ +350,5 @@
> +                FloatUtils.fuzzyEquals(newMetrics.viewportRectTop,
> +                                       newMetrics.pageRectTop) &&
> +                oldMetrics.fixedLayerMarginTop > 0) {
> +                newMetrics = newMetrics.setViewportOrigin(newMetrics.viewportRectLeft,
> +                                 newMetrics.pageRectTop - oldMetrics.fixedLayerMarginTop);

The fact that this adjusts the top only (not all the other edges) is specific to the hiding toolbar UI and doesn't really belong here in GLC. However the same is true for the other hunk (where you changed offsetY == 0 to fuzzyEquals(offsetY, pageTop)) and I allowed that so I'm not gonna r- this here. But at some point we should either move this code out or do the same for all the edges.

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> ::: mobile/android/base/gfx/GeckoLayerClient.java
> @@ +348,5 @@
> > +            // accordingly.
> > +            if (type == ViewportMessageType.UPDATE &&
> > +                FloatUtils.fuzzyEquals(newMetrics.viewportRectTop,
> > +                                       newMetrics.pageRectTop) &&
> > +                oldMetrics.fixedLayerMarginTop > 0) {
> 
> style nit: When formatting multi-line conditions like this I find it easier
> to read if the '&&' is at the start of the next line rather than at the end
> of the previous line. That way looking down the left margin the additional
> conditions don't look like statements inside the if-body.
> 
> i.e.
> if (type == ViewportMessageType.UPDATE
>     && FloatUtils.fuzzyEquals(...)
>     && ...) {
>     ...
> }
> 
> Even better would be an extra level of indentation to differentiate it more
> from the body:
> 
> if (type == ViewportMessageType.UPDATE
>         && FloatUtils.fuzzyEquals(...)
>         && ...) {
>     ...
> }

I actually think the opposite, that putting signs on the end of the line makes it easier to read. The indentation at the start indicates that it's a follow-on from the previous line, but having the sign at the end shows that it continues on the next line and you didn't just forget a semi-colon. That said, I'm apparently alone in this, so I'll make this change anyway :) Agree with the indentation.

> @@ +350,5 @@
> > +                FloatUtils.fuzzyEquals(newMetrics.viewportRectTop,
> > +                                       newMetrics.pageRectTop) &&
> > +                oldMetrics.fixedLayerMarginTop > 0) {
> > +                newMetrics = newMetrics.setViewportOrigin(newMetrics.viewportRectLeft,
> > +                                 newMetrics.pageRectTop - oldMetrics.fixedLayerMarginTop);
> 
> The fact that this adjusts the top only (not all the other edges) is
> specific to the hiding toolbar UI and doesn't really belong here in GLC.
> However the same is true for the other hunk (where you changed offsetY == 0
> to fuzzyEquals(offsetY, pageTop)) and I allowed that so I'm not gonna r-
> this here. But at some point we should either move this code out or do the
> same for all the edges.

Really, we only want to do it for the leading edge - I was going to implement this, but then you need to know if it's an RTL page, and I wasn't entirely sure how to do that, so I just gave up and stuck with the top edge.

Comment 4

[Chris Lord [:cwiiis]]

Amendment made and comment added about adjusting the left/right edges, pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/96fb830b930d

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96fb830b930d