[MP] Defect - Clicking the [X] on a download infobar should close it temporarily so it can still be toggled by the download button

VERIFIED FIXED in Firefox 27

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: emtwo, Assigned: emtwo)

Tracking

unspecified
Firefox 27
x86_64
Windows 8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [preview] feature=defect c=Downloads_app_bar u=metro_firefox_user p=1)

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Summary: Clicking the → Clicking the [X] on a download infobar should close it temporarily so it can still be toggled by the download button
Summary: Clicking the [X] on a download infobar should close it temporarily so it can still be toggled by the download button → Defect - Clicking the [X] on a download infobar should close it temporarily so it can still be toggled by the download button
Blocks: 910108
Duplicate of this bug: 910108
Priority: -- → P2
Summary: Defect - Clicking the [X] on a download infobar should close it temporarily so it can still be toggled by the download button → [MP] Defect - Clicking the [X] on a download infobar should close it temporarily so it can still be toggled by the download button
Whiteboard: [preview] feature=defect c=Downloads_app_bar u=metro_firefox_user p=0
I believe we only need to toggle the download-progress bar since I think when users click [X] on a download-request or download-complete bar they want to get rid of them whereas they may want to look at progress again.

I did this by recreating the progress notification when the download button is clicked if there are still downloads in progress

p=1
Assignee: nobody → msamuel
Status: NEW → ASSIGNED
Attachment #804007 - Flags: review?(mbrubeck)
Blocks: metrov1it15
No longer blocks: metrov1backlog
QA Contact: jbecerra
Whiteboard: [preview] feature=defect c=Downloads_app_bar u=metro_firefox_user p=0 → [preview] feature=defect c=Downloads_app_bar u=metro_firefox_user p=1
Comment on attachment 804007 [details] [diff] [review]
v1: After closing a download-progress infobar via [X], it may be toggled back on with the download button

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

::: browser/metro/base/content/downloads.js
@@ +441,5 @@
>            Downloads._downloadProgressIndicator.reset();
> +        } else if (aEvent.notification.value == "download-progress" &&
> +            Downloads._progressNotification != null) {
> +          // Closing a download-progress notification via [X] 'toggles' it off.
> +          Downloads._notificationBox.notificationsHidden = true;

Do we need to use notificationsHidden here, since the alert has been removed already?  It seems weird that dismissing the download progress notification would also hide other active notifications.  I'm worried that we can leave the notificationbox in a state where new notifications won't appear -- even unrelated ones like geolocation or popup permissions.  (Sorry for not noticing this in previous reviews.)

I like that this patch lets us just recreate the progress notification -- could we completely switch from notificationsHidden to destroying/creating (or hiding/showing) just this one notification?
Attachment #804007 - Flags: review?(mbrubeck) → review-
Instead of notificationbox.notificationsHidden, appbar keeps track of visible download notifications in a map so that it can create and destroy them instead of hiding.

I just wanted to get input so far if this is on the right track. Based on the tests I've done so far it seems to be working as expected but I will test some more.
Attachment #804007 - Attachment is obsolete: true
Attachment #806809 - Flags: feedback?(mbrubeck)
Attachment #806809 - Attachment is obsolete: true
Attachment #806809 - Flags: feedback?(mbrubeck)
Attachment #807210 - Flags: review?(mbrubeck)
Comment on attachment 807210 [details] [diff] [review]
v3: Toggle download-progress notifications

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

Looks great, thanks!  One minor nit:

::: browser/metro/base/content/appbar.js
@@ +88,5 @@
>      this._updateStarButton();
>    },
>  
>    onDownloadButton: function() {
> +    Downloads.onDownloadButton();

Let's just get rid of Appbar.onDownloadButton, and have browser.xul reference Downloads.onDownloadButton directly.

Note: bug 875731 is renaming "Downloads" to "MetroDownloadsView" to avoid conflict with a toolkit module, so this patch will need to be updated if that one lands first.
Attachment #807210 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/integration/fx-team/rev/9f2d24b55cea
Whiteboard: [preview] feature=defect c=Downloads_app_bar u=metro_firefox_user p=1 → [fixed-in-fx-team][preview] feature=defect c=Downloads_app_bar u=metro_firefox_user p=1
https://hg.mozilla.org/mozilla-central/rev/9f2d24b55cea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][preview] feature=defect c=Downloads_app_bar u=metro_firefox_user p=1 → [preview] feature=defect c=Downloads_app_bar u=metro_firefox_user p=1
Target Milestone: --- → Firefox 27
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0

Verified as fixed on the latest nightly (Build ID: 20131003030203) during iteration 15 testing.
The download bar is closed by clicking the [X] button on a download info bar (the download continues). Also, it can be toggled by clicking the download button.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.