Closed Bug 898877 Opened 7 years ago Closed 7 years ago

Dynamic toolbar hides when page fits the screen

Categories

(Firefox for Android :: Toolbar, defect)

23 Branch
ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox22 --- unaffected
firefox23 - wontfix
firefox24 + verified
firefox25 + verified
firefox26 --- verified
fennec 23+ ---

People

(Reporter: mkajda, Assigned: kats)

References

Details

(Keywords: reproducible)

Attachments

(2 files)

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).
OS: Linux → Android
Hardware: x86_64 → ARM
I can reproduce this on LG Nexus 4 (Android 4.2.2), Firefox for Android (2013-07-28)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Is it the same as Bug 886576? 
Although I'm not able to reproduce this with the twitter page.
Sounds like bug 866772 is still around.
tracking-fennec: --- → ?
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.
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.
Keywords: qawanted
Summary: Awesome Bar hides when page fits the screen → Dynamic toolbar hides when page fits the screen
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
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.
Flags: needinfo?(mark.finkle)
Looking at the changelog bug 858969 seems like the best candidate.
Comment 5 is a "triage" comment. The suspicion was from Brad, but as Kevin points out, this is likely something else.
Flags: needinfo?(mark.finkle)
Adding regressionwindow-wanted to find out when the fix window was.
The regression window exists, comment 6 and comment 8.
(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 ?
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.
Flags: needinfo?(mihai.g.pop)
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)
Is 24 affected?
Keywords: qawantedreproducible
Yes, I can reproduce on Firefox 24 Beta 1 using both STRs.
Assignee: nobody → bugmail.mozilla
tracking-fennec: ? → 23+
Blocks: 902138
Flags: needinfo?(mihai.g.pop)
Attached patch Part 1 - The fixSplinter Review
This should fix this behaviour. At least it does on various test pages I tried.
Attachment #790861 - Flags: review?(chrislord.net)
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.
Attachment #790864 - Flags: review?(chrislord.net)
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.
Attachment #790861 - Flags: review?(chrislord.net) → review+
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.
Attachment #790864 - Flags: review?(chrislord.net) → review+
(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 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.
Attachment #790861 - Flags: approval-mozilla-beta?
Attachment #790861 - Flags: approval-mozilla-aurora?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 on attachment 790861 [details] [diff] [review]
Part 1 - The fix

Approving and requesting QA verification once this lands.
Attachment #790861 - Flags: approval-mozilla-beta?
Attachment #790861 - Flags: approval-mozilla-beta+
Attachment #790861 - Flags: approval-mozilla-aurora?
Attachment #790861 - Flags: approval-mozilla-aurora+
Keywords: verifyme
https://hg.mozilla.org/mozilla-central/rev/ae2f8afea260
https://hg.mozilla.org/mozilla-central/rev/f9d39959fc55
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Status: RESOLVED → VERIFIED
Keywords: verifyme
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)
You need to log in before you can comment on or make changes to this bug.