898877 - Dynamic toolbar hides when page fits the screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Michal Kajda [:mkajda]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130627172038

Steps to reproduce:

1. Scale web page so that its height fits the height of the screen.
2. Slide up to hide Awesome Bar.

It is possible to reproduce it for many pages (for example google.com), but it may not be easy.
You can reproduce it for 100% for Nexus 4 (Android 4.3, tested on Nightly 2013-07-28 and Beta):
1. Keep your phone in portrait orientation.
2. Go to http://reynolds-cs.com/get-in-touch/
3. Zoom out page to its minimum (it will adjust to fit Nexus' screen)
4. Slide up to hide Awesome Bar
5. Now web page should fit screen perfectly and it's impossible to pan it vertically to show Awesome Bar.

You need to zoom a page in/out or put Firefox to background and bring to foreground again to show Awesome Bar.
It's not very big deal but I consider it as a good reason to file a bug.
Interestingly, this doesn't occur again when you sent Firefox to background and brought to foreground after encountering this issue. You need to reload the web page to test it again.



Actual results:

Web page's height fits to the height of the screen so that it is impossible to slide it vertically and make Awesome Bar visible again.




Expected results:

Awesome Bar should not disappear when page's height fits to screen.
This occurs for beta and nightly (2013-07-28), but doesn't for 22.0 (at least I wasn't able to reproduce it).

Comment 1

[Teodora Vermesan (:TeoVermesan)]

I can reproduce this on LG Nexus 4 (Android 4.2.2), Firefox for Android (2013-07-28)

Comment 2

[Teodora Vermesan (:TeoVermesan)]

Is it the same as Bug 886576? 
Although I'm not able to reproduce this with the twitter page.

Comment 3

[Brian Nicholson (:bnicholson)]

Sounds like bug 866772 is still around.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

If we can find a regression window and perhaps more sites that are affected and highly reproducible then hopefully we can get more attention on this but since we're about to build our final beta there's nothing to block release here at this time without a sense of wider user impact and pain that would necessitate a respin of the mobile build.  If this does start to look like a larger issue please contact relman immediately.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

If the test page is consuming touch events, it's likely a dupe of bug 866793.

If this a a situation where the user is pinch zooming to get the page to fit, then can't pan the dynamic toolbar, it's a separate bug.

Comment 6

[u421692]

The regression window is:

1.mozilla-central
good build: 24.04.2013 
bad build: 25.04.2013 
-pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fef5f202b2dc&tochange=690b5e0f6562

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Mark - does this range show something that agrees with your suspicion to dupe this to bug 866793?  Nothing stands out in that range from my read-over.

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

Looking at the changelog bug 858969 seems like the best candidate.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment 5 is a "triage" comment. The suspicion was from Brad, but as Kevin points out, this is likely something else.

Comment 10

[Alex Keybl [:akeybl]]

Adding regressionwindow-wanted to find out when the fix window was.

Comment 11

[Kevin Brosnan [Ex-Mozilla]]

The regression window exists, comment 6 and comment 8.

Comment 12

[bhavana bajaj [:bajaj]]

(In reply to Kevin Brosnan [:kbrosnan] from comment #11)
> The regression window exists, comment 6 and comment 8.

Given Bug 858969  is fixed in 23,24 is affected as well ?

Comment 13

[Kevin Brosnan [Ex-Mozilla]]

858969 caused this bug from what Mihai found. I would guess so. I've not been not able to reproduce this issue. 

Mihai could you verify that that the other versions are affected.

Comment 14

[Teodora Vermesan (:TeoVermesan)]

I'm able to reproduce this on Firefox for Android 25.0a2 (2013-08-12) and on Firefox for Android 26.0a1 (2013-08-12) using LG Nexus 4 (Android 4.2.2 )

I found another STR:
1. Go to https://addons.mozilla.org/en-US/android
2. Tap on Phony
3. Tap the reader icon to enter reader mode
4. Slide up to hide Awesome Bar
5. Try to pan vertically to show Awesome Bar.

Note: 
Device: Acer Iconia 
OS: Android 3.2 
Build: Firefox for Android 25.0a2 (2013-08-12) and Firefox for Android 26.0a1 (2013-08-12)

Comment 15

[Aaron Train [:aaronmt]]

Is 24 affected?

Comment 16

[Teodora Vermesan (:TeoVermesan)]

Yes, I can reproduce on Firefox 24 Beta 1 using both STRs.

Comment 17

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

Attachment: Part 1 - The fix

This should fix this behaviour. At least it does on various test pages I tried.

Comment 18

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

Attachment: Part 2 - Unrelated fix

While poking around the code I discovered this which seemed wrong to me. So I fixed it. I'm not sure if there are pages that exercise this bug but it makes more sense this way.

Comment 19

[Chris Lord [:cwiiis]]

Comment on attachment 790861 [details] [diff] [review]
Part 1 - The fix

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

This seems like a reasonable fix - I thought that our code reduced the size of the viewport unless the page was *greater* than the size of the screen, rather than greater-than-or-equal, but I guess it doesn't? Even if that is what it's supposed to do, this seems like a good idea as a fail-safe anyway.

Comment 20

[Chris Lord [:cwiiis]]

Comment on attachment 790864 [details] [diff] [review]
Part 2 - Unrelated fix

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

LGTM. See other review comment about when the viewport is expanded, but I'll leave it up to whatever you think is best.

Comment 21

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

(In reply to Chris Lord [:cwiiis] from comment #19)
> This seems like a reasonable fix - I thought that our code reduced the size
> of the viewport unless the page was *greater* than the size of the screen,
> rather than greater-than-or-equal, but I guess it doesn't?

Yeah based on the condition at [1] the viewport size is larger if the page is greater-than-or-equal-to the screen size. Based on our IRC discussion I will keep this as-is. (In summary: the code matches the comments above, and this behaviour seems more correct to me).

[1] http://hg.mozilla.org/mozilla-central/file/6f265af4e3d8/mobile/android/chrome/content/browser.js#l3879

Comment 22

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

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

Comment 23

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

Comment on attachment 790861 [details] [diff] [review]
Part 1 - The fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): original dynamic toolbar implementation, i think
User impact if declined: sometimes you get stuck with the dynamic toolbar off the screen and can't get it back
Testing completed (on m-c, etc.): locally so far
Risk to taking this patch (and alternatives if risky): affects fennec only, but there's an uplift risk because i'm not sure what the relevant code looks like on 25 and 24 and some work may be needed to rebase/apply the patch there.
String or IDL/UUID changes made by this patch: none

Note that I'm only looking to uplift the first patch here since that is the actual fix. The second patch can ride the trains unless we find that there is something specific it fixes.

Comment 24

[bhavana bajaj [:bajaj]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> Comment on attachment 790861 [details] [diff] [review]
> Part 1 - The fix
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): original dynamic toolbar
> implementation, i think
> User impact if declined: sometimes you get stuck with the dynamic toolbar
> off the screen and can't get it back
> Testing completed (on m-c, etc.): locally so far
> Risk to taking this patch (and alternatives if risky): affects fennec only,
> but there's an uplift risk because i'm not sure what the relevant code looks
> like on 25 and 24 and some work may be needed to rebase/apply the patch
> there.
> String or IDL/UUID changes made by this patch: none

Given we are still have a couple of beta's I am approving patches for beta/aurora.We'll backout if we have any worse regressions or unexpected fallouts.PLease mkae sure to have the rebased patches landed by MOnday which is when our next beta goes to build.
> 
> Note that I'm only looking to uplift the first patch here since that is the
> actual fix. The second patch can ride the trains unless we find that there
> is something specific it fixes.

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 790861 [details] [diff] [review]
Part 1 - The fix

Approving and requesting QA verification once this lands.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ccb6826afa7
https://hg.mozilla.org/releases/mozilla-beta/rev/ee4d263ee510

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ae2f8afea260
https://hg.mozilla.org/mozilla-central/rev/f9d39959fc55

Comment 28

[u421692]

Verified as fixed on:
Device: LG Nexus 4 (Android 4.2.2)
Build: Firefox 24 Beta 6 (2013-08-26) / Firefox 25 Aurora (2013-08-27)