953430 - tapping on completed download button under about:start produces a blank app bar

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Downloads, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: Extra App Bar that's blank

When you tap on the completed download button under the about:start screen, you'll get a blank app bar above the app bar that presents users with the "Open", "Open in folder" and "X" buttons.

I can reproduce this with both Nightly 29 and Aurora 28 (builds listed below)

- Attached a screenshot to illustrate the issue

Steps to reproduce the issue:

1) Open Firefox Metro (either Nightly or Aurora)
2) Download something from any website (I used VLC in this case)
3) Once the download is completed, close your current tab and you should be re-directed to the about:start screen
4) Notice that the download button is still visible, tap on the download button and you'll see the blank app bar above the first app bar

Current Behavior:

- When a user taps on the completed download button under the about:start screen, they will get two app bars. The one at the top will be completely blank

Expected Behavior:

- When a user taps on the completed download button, they should only receive a single app bar with "Open", "Open in folder" and "X" options.

Used the following builds to find the issue:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-28-03-02-04-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-29-00-40-02-mozilla-aurora/

Comment 1

[Matt Brubeck (:mbrubeck)]

I'm planning to work on this in the current iteration, p=2.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This happened because we had two separate ways to move the browser above the navbar on about:start.  In browser.css we set "padding-bottom" on the <browser>, and in ContentAreaObserver we set the "height" of the <notificationbox>.  When both were applied at once, it doubled the padding.

This patch makes both rules set "padding-bottom" on the <notificationbox>, so the results will be be the same whether one or both are set.

After making that change, I also found that the style wasn't always removed at the correct time, because ContentAreaObserver.update would bail out if the window size hadn't changed.  So this patch also makes the notification check in updateContentArea run unconditionally.

Comment 3

[Marina Samuel [:emtwo]]

Comment on attachment 8359525 [details] [diff] [review]
patch

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

Just one comment for something to consider but otherwise looks good!

::: browser/metro/theme/browser.css
@@ +188,5 @@
>  }
>  
>  /* Start UI ----------------------------------------------------------------- */
>  
> +#content-viewport[startpage] .active-tab-notificationbox {

I'm wondering if we could completely get rid of this (and the changes in browser.js) since now we set padding-bottom in ContentAreaObserver.js?

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Marina Samuel [:emtwo] from comment #3)
> I'm wondering if we could completely get rid of this (and the changes in
> browser.js) since now we set padding-bottom in ContentAreaObserver.js?

Yeah, I think we should clean up this code, though I'd like to stick with this minimal change for now to try to minimize any risk of new regressions on Aurora.  I'd kind of like to move more of this into CSS, by setting attributes that we can determine entirely with CSS selectors whether the padding should be present.

Comment 5

[Matt Brubeck (:mbrubeck)]

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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2783ed7e977d

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8359525 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Metro Firefox

User impact if declined: Cosmetic glitch on start page during file download.

Testing completed (on m-c, etc.): Landed on m-c on 01-14.

Risk to taking this patch (and alternatives if risky): Low-risk, Metro-only.

String or IDL/UUID changes made by this patch: None.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[u279076]

Kamil, please verify this is fixed in the latest Nightly and Aurora builds.

Comment 10

[Kamil Jozwiak [:kjozwiak]]

Went through the following issue for verification during iteration #22. Used the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-01-25-00-40-03-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-01-25-03-02-05-mozilla-central/

- Went through the original test case from comment #0 and ensured that the extra white app bar doesn't appear above the "Download App Bar"
- Ensured that taping/clicking on the "Download Complete" button correctly slides in the "Download App Bar" without any issues
- Ensured that all the buttons available under the "Download App Bar" are working correctly before the download is initialized (Open, Save, Cancel, "X")
- Ensured while the download is in progress, taping/clicking on the "X" dismisses the "Download App Bar" but doesn't cancel the download
- Ensured while the download is in progress, taping/clicking on "Cancel" correctly cancels the download and dismisses the "Download App Bar" (ensured that the download button isn't visible after the cancellation)
- Ensured that taping/clicking on "Show in Files" switches to the windows environment and opens the "download" path using File Explorer
- Ensured that the "Download App Bar" slides into view when the download is completed if the "Navigation App Bar" isn't visible
- Ensured that you can browse while the download is in progress without any issues
- Went through all of the above test cases using different variations of snapped view

Found the following issues when going through this issue:
- Bug 964002