896547 - Dynamic toolbar causes scroll events to be fired repeatedly

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Botond Ballo [:botond]]

To reproduce, open http://people.mozilla.org/~bballo/plain.html (plain HTML page with a wide image), with the dynamic toolbar enabled. The scrollbar on the right flickers repeatedly.

It looks like a scroll event is being fired repeatedly, which shouldn't be happening.

Comment 1

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

This is a more recent regression than the original dynamic toolbar code. It only happens on nightly.

Comment 2

[Teodora Vermesan (:TeoVermesan)]

The regression window is:
1. mozilla-central:
good build: 11.07.2013 
bad build:  12.07.2013 
-pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dde4dcd6fa46&tochange=b44898282f21

2. inbound:
good build: 1373557796
bad build:  1373562059 
-pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=df4cc5d63c7d&tochange=9e018ae9cbed

Comment 3

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

Thanks.

Comment 4

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

Note that this still doesn't happen on aurora or beta, so it must be caused by the second (refactoring) patch on bug 877602 which was not uplifted.

Comment 5

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

Attachment: Patch

Try push at https://tbpl.mozilla.org/?tree=Try&rev=7362360d9a84

Comment 6

[Chris Lord [:cwiiis]]

Comment on attachment 788960 [details] [diff] [review]
Patch

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

LGTM, I guess. Sorry to have roped you into this :)

::: mobile/android/chrome/content/browser.js
@@ +3916,5 @@
> +
> +    // Store the page size that was used to calculate the viewport so that we
> +    // can verify it's changed when we consider remeasuring in updateViewportForPageSize
> +    let viewport = this.getViewport();
> +    this.lastPageSizeUsedForViewportChange = {

Maybe this should be renamed lastPageSizeAfterViewportRemeasure, perhaps? It doesn't feel like this comment really corresponds to what's actually happening anymore...

Comment 7

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

I updated the variable name as you suggested. The comment still made sense to me so I left it.

https://hg.mozilla.org/integration/fx-team/rev/ec6710da612e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec6710da612e

Comment 9

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

Comment on attachment 788960 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 877602 (second patch)
User impact if declined: on some pages the code will continue doing layouts infinitely, which is bad for battery life. also the scrollbars will keep flashing.
Testing completed (on m-c, etc.): locally; this should bake on m-c for a few days to ensure there's no other fallout
Risk to taking this patch (and alternatives if risky): low risk, fennec-only code
String or IDL/UUID changes made by this patch: none

Comment 10

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 26.0a1 (2013-08-13)
Device: HTC Desire HD
OS: Android 2.3.5

Comment 11

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/d743c464c9cc

Comment 12

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0a2 (2013-08-27)
Device: HTC Desire HD
OS: Android 2.3.5