Download notification disappears after a download link opens in a new tab

VERIFIED FIXED in Firefox 28

Status

defect
P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: yfdyh000, Assigned: sfoster)

Tracking

Trunk
Firefox 30
Dependency tree / graph

Firefox Tracking Flags

(firefox28 verified, firefox29 verified, firefox30 verified)

Details

(Whiteboard: [release28] p=3 s=it-30c-29a-28b.3 r=ff28)

Attachments

(7 attachments, 7 obsolete attachments)

2.71 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
3.50 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
5.70 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
8.03 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
5.26 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
28.22 KB, patch
Details | Diff | Splinter Review
27.45 KB, patch
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.3; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140123185438

Steps to reproduce:

For example, open http://360.cn/, click yellow 'Download' button at center, it linked to http://down.360safe.com/inst.exe.


Actual results:

Firefox automatically open a new tab to open this file (some mechanism?), the download notification emergence (moment), then quickly the blank page be automatically close, the notification disappear with the page.


Expected results:

Keep downloads notification, even if the page has closed.
Or identifying a blank page for downloads, and the download notification be transferred to the parent page.
Whiteboard: [defect] p=0
Whiteboard: [defect] p=0 → [triage] [defect] p=0
Whiteboard: [triage] [defect] p=0 → [release28] p=0
Whiteboard: [release28] p=0 → [release28] p=3
Whiteboard: [release28] p=3 → [release28] p=3 r=ff28
Hey Kamil, can you confirm this bug is a defect.  If it is, you can change the status to new.  Thanks.
Flags: needinfo?(kamiljoz)
I went through the test case that was added in comment #0 and reproduced the problem. Every time you click on the "Yellow Button", a new blank tab appears with the "Download App Bar" sliding into view. After a second or so, the "Download App Bar" disappears. Reproduced the issue with the following builds:

- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-08-03-02-07-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-08-00-40-02-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/win32/en-US/

Thanks for posting Yang!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(kamiljoz)
The yellow button is just a styled anchor, pointing at the file for download. I'm guessing somewhere in that minified script an event listener targets all link clicks at a new window. When the window opens and it gets the binary response it begins the download process and closes the empty tab. We briefly see the save download notification, but the tab is closed before the user can get to it. 

We have code at http://dxr.mozilla.org/mozilla-central/source/browser/metro/base/content/downloads.js#550 which handles this case for in-progress downloads. It should be extended to do the same thing for the download-start notification.
Assignee: nobody → sfoster
status update: turns out that we rescue the in-progress notifications from tabs before they get closed, but not the start and completed notifications. I'm polishing a patch to fix this and ensure visible downloads notifications seamlessly transfer to the next selected tab.
Whiteboard: [release28] p=3 r=ff28 → [release28] p=3 r=ff28 s=it-30c-29a-28b.1
So it turns out that the always-slightly-dubious mechanism for "rescuing" notifications before the tab they are associated with is destroyed - is in fact a problem. In order to fix this issue properly we need to move the "save-download" notification into the nextTab. In my attempt to extend the TabClose handler to do this, I find that the notification label gets lost in-transit, and there are other side-effects to not using the appendNotification notifictationbox method - which creates the notification, passes it into _showNotification and fires off a AlertActive event. 

I see a couple of options: 
* Find out why the label is not showing up and implement the hopefully-simple fix for just that issue and leave the rest alone. If this can be done, it keeps the scope of this bug down, but leaves the bomb ticking. In particular, I suspect not firing the AlertActive event is a bad thing. We could do that in our downloads.js too, but this code duplication *will* bite us eventually. 

* Refactor the notificationbox to make it possible to pass in a notification node to adopt and show. This will need to be ok'd by the desktop teeam, has risks and will require all desktop notifications to be re-verified. 

* Replace the DOM manipulation mechanism for moving notifications with  one that re-creates the notifications using the existing appendNotification method. We'll need to refactor HelperAppDialog a bit to accomplish this - which is where the download-start notification is currently constructed. This will need some care to make sure notifications go back in in the right order and will likely introduce some dependency relationship between HelperAppDialog and MetroDownloadsView. There will also be some perf cost as we recreate and re-bind those notifications, but it may not be significant

Thoughts?
Attachment #8376398 - Flags: feedback?(mbrubeck)
Comment on attachment 8376398 [details] [diff] [review]
WIP: move all downloads notifications to nextTab when the selected tab closes

I think we should try whatever short-term fix we can get to work with minimal code/effort, and work on adding a global notification area (like in desktop Firefox) for the long term so we can stop migrating notifications between tabs.  Unfortunately I'm not sure which of the short-term options is best...

An alternate short-term fix possibility: stop the new tab from auto-closing, at least until the notification is gone.
Attachment #8376398 - Flags: feedback?(mbrubeck)
Moving notifications from one notificationbox to another causes the binding to be unapplied then applied. This was blowing away the markup used by some notification labels. 
This patch creates a new notification binding which extends toolkit's to provide a setter and getter for the label property to make it simple to assign a complex label value as a documentfragment or other node
Attachment #8376605 - Flags: review?(mbrubeck)
Move all download notifications to the next tab when the current tab closes. 
The only outstanding issue I see here is when the next tab is about:start, the notification is correctly positioned, but closing it does not trigger CAO.update, so the notificationbox retains its padding-bottom leaving a white stripe above the navbar. I dont see how this is working in other cases but not here
Attachment #8376398 - Attachment is obsolete: true
Attachment #8376606 - Flags: feedback?(mbrubeck)
Attachment #8376606 - Flags: feedback?(jmathies)
Status: NEW → ASSIGNED
Priority: -- → P1
QA Contact: jbecerra
Whiteboard: [release28] p=3 r=ff28 s=it-30c-29a-28b.1 → [release28] p=3 r=ff28
Comment on attachment 8376605 [details] [diff] [review]
Extended notification binding to provide child element support for the notification label (part 1 of 2)

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

::: browser/metro/base/content/bindings/notification.xml
@@ +86,5 @@
> +            let frag = this.ownerDocument.createDocumentFragment();
> +            let containerNode = this._messageContainer;
> +            let cnode;
> +            while((cnode = containerNode.firstChild)) {
> +              frag.appendChild(cnode);

Is this destructive?  (I.e., does it remove the label content from its original container?)  If so, I'd like to either (a) add some cloneNode calls to fix that, or (b) change this from a getter to a method with a name that makes it clear what is happening.

@@ +100,5 @@
> +            if (val && "object" == typeof val && 'nodeType' in val) {
> +              let containerNode = this._messageContainer;
> +              let cnode;
> +              while((cnode = containerNode.firstChild)) {
> +                containerNode.removeChild(cnode);

You can use the new "cnode.remove()" method here instead.  (No semantic difference; just shorter.)
Attachment #8376605 - Flags: review?(mbrubeck) → review+
Comment on attachment 8376606 [details] [diff] [review]
move all download notifications to next tab when current tab gets closed (2 of 2)

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

::: browser/metro/base/content/downloads.js
@@ +550,5 @@
> +        try {
> +
> +          let browser = aEvent.originalTarget.linkedBrowser;
> +          let tab = Browser.getTabForBrowser(browser);
> +          let tabIndex = Browser._tabs.indexOf(tab);

tabIndex appears to be unused.

@@ +576,5 @@
> +
> +              // Fire event for accessibility APIs
> +              var event = document.createEvent("Events");
> +              event.initEvent("AlertActive", true, true);
> +              notn.dispatchEvent(event);

AlertActive will trigger ContentAreaObserver.updateContentArea, but this won't get the correct new height because the next tab is not selected yet.  I think this is the cause of the problem in comment 8.

We could maybe fix this by also calling updateContentArea on TabSelect events, in addition to AlertActive and AlertClose:
http://hg.mozilla.org/mozilla-central/file/6e3ec93efe1d/browser/metro/base/content/ContextUI.js#l366
Attachment #8376606 - Flags: feedback?(mbrubeck) → feedback+
(In reply to Sam Foster [:sfoster] from comment #8)
> Created attachment 8376606 [details] [diff] [review]
> move all download notifications to next tab when current tab gets closed (2
> of 2)
> 
> Move all download notifications to the next tab when the current tab closes. 
> The only outstanding issue I see here is when the next tab is about:start,
> the notification is correctly positioned, but closing it does not trigger
> CAO.update, so the notificationbox retains its padding-bottom leaving a
> white stripe above the navbar. I dont see how this is working in other cases
> but not here

I see this all the time on the save password notification.
How should I go about testing this? downloading a large download and closing tabs? should everything work if there's only one tab open?
QA Contact: jbecerra → kamiljoz
Whiteboard: [release28] p=3 r=ff28 → [release28] p=3 s=it-30c-29a-28b.2 r=ff28
(In reply to Jim Mathies [:jimm] from comment #11)
> (In reply to Sam Foster [:sfoster] from comment #8)
> > Created attachment 8376606 [details] [diff] [review]
> > move all download notifications to next tab when current tab gets closed (2
> > of 2)
> > 
> > Move all download notifications to the next tab when the current tab closes. 
> > The only outstanding issue I see here is when the next tab is about:start,
> > the notification is correctly positioned, but closing it does not trigger
> > CAO.update, so the notificationbox retains its padding-bottom leaving a
> > white stripe above the navbar. I dont see how this is working in other cases
> > but not here
> 
> I see this all the time on the save password notification.

filed bug 974413 on this.
(In reply to Jim Mathies [:jimm] from comment #12)
> How should I go about testing this? downloading a large download and closing
> tabs? should everything work if there's only one tab open?

I normally test by opening about:start in a tab, then opening http://nightly.mozilla.org/ in a new tab and clicking on one of the downloads. It puts up the save or run notification - do nothing, just close that tab. With the patch applied, you should see the notification moved as-is to that first about:start tab.  The navbar should be visible with the notification above. If you cancel the download it /should/ slide away leaving no trace, but currently leaves the white stripe. 

(Note that if you closed your last tab we create a new about:start tab - in that case we don't currently expect notifications to be preserved. We would need to create a special notification limbo for them to exist in while we create the new notificationbox and browser. I think I'd rather figure out a global notifications thing than attempt to fix this with the way we do tabs and notificationbox now)
Builds on the previous version of this patch (attachment 8376605 [details] [diff] [review]) to add an adoptNotification to our binding, moving that logic and code-duplication out of MetroDownloadsView. This way we can call the _showNotification and do all the things that appendNotification does apart from actually creating the elements -which has already been done. 

The previous version was r+'d by mbrubeck. No particular feedback needed on this, its just what the next patch layers onto. Just let me know if something jumps out at you
Attachment #8376605 - Attachment is obsolete: true
Attachment #8378509 - Flags: feedback?(jmathies)
Uses the new adoptNotification method for a cleaner and more complete implementation of what we want to happen here - that the notification should move to the next tab's notificationbox, but should otherwise by identical to normal which is calling appendNotification to create and show the notification. 

Note that this all seems to work great. But when you dimiss the notification, the content area is not resized to take up the notificationbox height; you get a white strip where it was, and if you're using the mouse, you'll see the horizontal scrollbars stay put (so its not just a painting issue). That issue is a bit orthogonal to this bug, but a regression nonetheless. Thoughts - both on that issue and how to proceed?
Attachment #8376606 - Attachment is obsolete: true
Attachment #8376606 - Flags: feedback?(jmathies)
Attachment #8378512 - Flags: feedback?(jmathies)
Attachment #8378512 - Attachment is patch: true
Attachment #8378512 - Attachment mime type: message/rfc822 → text/plain
Comment on attachment 8378509 [details] [diff] [review]
Extended notification binding, adds adoptNotification and child element support for the notification label (part 1 of 2)

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

Appears to work as expected. I tried testing with a geo location prompt in a base tab and download in second tab. Then I closed the download tab and noted that there were two notifications in the base tab both of which i could respond to.

I think a test for this important.
Attachment #8378509 - Flags: feedback?(jmathies) → feedback+
Comment on attachment 8378512 [details] [diff] [review]
move all download notifications to next tab when current tab gets closed (2 of 2)

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

::: browser/metro/base/content/ContextUI.js
@@ +366,5 @@
>          break;
>        case "AlertActive":
>        case "AlertClose":
> +      case "TabSelect":
> +        dump("ContextUI, handling event "+aEvent.type+" and calling CAO.updateContentArea\n");

nit - I'd prefer Util.dumpLn here and in other places.
Attachment #8378512 - Flags: feedback?(jmathies) → feedback+
A couple of tests for the extended notification bindings added in attachment 8378509 [details] [diff] [review] and used by attachment 8378512 [details] [diff] [review]. You'll need both those in your patch queue ahead of this one. The tests should a) pass, and b) test enough to catch serious regressions (without unduly hindering future refactoring of how we handle notifications)
Attachment #8381808 - Flags: review?(azasypkin)
A tests for the handling of download notifications when the current tab gets closed. As we dont have global notifications, MetroDownloadsView listens for TabClose events and attempt to move any active download-related notifications to the next tab. You'll need both attachment 8378509 [details] [diff] [review] and attachment 8378512 [details] [diff] [review] in your queue to run the test. 

You'll notice all the other downloads tests are currently todos, so the assertions in this new test are the only ones that should report a result.
Attachment #8381814 - Flags: review?(azasypkin)
Comment on attachment 8381808 [details] [diff] [review]
Tests for the notification bindings and adoptNotification functionality

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

Looks good, just a few minor comment below. Regarding to coverage, was thinking about testing priority, but just noticed that the following patch deals with it.

Btw, do we have any js coverage tool that is going to work with our codebase?

::: browser/metro/base/tests/mochitest/browser_notifications.js
@@ +24,5 @@
> +    let notn = createTestNotification();
> +
> +    let binding = notn && getComputedStyle(notn).MozBinding;
> +    is(binding,
> +       'url("chrome://browser/content/bindings/notification.xml#notification")',

Can we use double quotes for strings everywhere just to be consistent? I know "url(\"chrome://browser/content/bindings/notification.xml#notification\")" looks not so nice, but consistent :)

@@ +65,5 @@
> +
> +gTests.push({
> +  desc: "Check adoptNotification does what we expect",
> +  setUp: function() {
> +    yield addTab("about:mozilla");

Just curious, do we need to close tabs on teardown or we shouldn't care about it? Some of our tests close created tabs, some don't.
Attachment #8381808 - Flags: review?(azasypkin) → review+
Attachment #8381814 - Flags: review?(azasypkin) → review+
Depends on: 974413
Blocks: 977364
Summary: Can not download file in different domain → Download notification disappears after a download link opens in a new tab
Whiteboard: [release28] p=3 s=it-30c-29a-28b.2 r=ff28 → [release28] p=3 s=it-30c-29a-28b.3 r=ff28
Updating patch, carrying Oleg's r+
Attachment #8381814 - Attachment is obsolete: true
Attachment #8385685 - Flags: review+
Uses new adoptNotification notificationbox method to move notifications to the next tab's notificationbox when a tab closes.
Attachment #8378512 - Attachment is obsolete: true
Attachment #8385687 - Flags: review?(mbrubeck)
Patch update, carrying azasypkin's r+
Attachment #8381808 - Attachment is obsolete: true
Attachment #8385688 - Flags: review+
We were previously special-casing the start page. This patch applies the necessary padding to accommodate both notifications and navbar whenever both are visible, by updating a chromeState broadcaster and its navbar="visible" | navbar="" state attribute, which lets us apply padding via a CSS rule.
Attachment #8385692 - Flags: review?(mbrubeck)
In order to keep our rich labels, turns it we need to override the toolkit widget's label setter and getter. This patch adds a notification binding for metro to do (just) that. To move the notification management code out of MetroDownloadsView, a new adoptNotification method was added to our notificationbox. This does the same work as appendNotification, but without creating the notification element.
Attachment #8378509 - Attachment is obsolete: true
Attachment #8385697 - Flags: review?(mbrubeck)
I've added the patches as a series of 5 here. They build on attachment 8385672 [details] [diff] [review] from bug 974413, which you'll need in your queue to test properly. Let me know if you would rather I just folded these 5 up together for review and landing as one unit. The test patches are largely unchanged (just unborked the tearDown) since review so I've carried r+, though it wouldn't hurt to look them over. 

This all came up green on try: https://tbpl.mozilla.org/?tree=Try&rev=ec0a22fd3ca3
Attachment #8385697 - Flags: review?(mbrubeck) → review+
Attachment #8385692 - Flags: review?(mbrubeck) → review+
Comment on attachment 8385687 [details] [diff] [review]
4 of 5 - Move all download notifications to next tab when current tab gets closed

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

::: browser/metro/base/content/downloads.js
@@ +567,5 @@
> +          if (nextBox) {
> +            // move notification to the next tab
> +            let label = notn.label;
> +            nextBox.adoptNotification(notn);
> +            notn.label = label;

Could the .label shuffling be moved inside of adoptNotification?
Attachment #8385687 - Flags: review?(mbrubeck) → review+
For testing and verification.  Reopen if any defects found.
Flags: needinfo?(kamiljoz)
I got these patches working on a mozilla-beta checkout, and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3448bea61915

Locally, it seems to check out: mochitest-metro came up clean and the fix looks good as far as our STR go.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Defect in original implementation of the download notifications feature
User impact if declined: User will not be able to open, cancel or track download progress once the originating tab is closed. 
Testing completed (on m-c, etc.): Verified on m-c, try push against beta was green
Risk to taking this patch (and alternatives if risky): Low risk, metro-only
String or IDL/UUID changes made by this patch: None

This is the patch set that I pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=3448bea61915, qfolded up. It includes a couple of merge conflict resolutions necessary to merge cleanly onto beta/28
Attachment #8386555 - Flags: approval-mozilla-beta?
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Defect in original implementation of the download notifications feature
User impact if declined: User will not be able to open, cancel or track download progress once the originating tab is closed. 
Testing completed (on m-c, etc.): Verified on m-c
Risk to taking this patch (and alternatives if risky): Low risk, metro-only
String or IDL/UUID changes made by this patch: None

Just one white-space/line-wrap merge conflict resolved to get the m-c changeset to apply to aurora
Attachment #8386584 - Flags: approval-mozilla-aurora?
Attachment #8386584 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8386555 [details] [diff] [review]
Combined patch, with margin fix from 974413 and parts 1-5 adapted to merge into beta/28

We'll take this Metro-only fix even though it's not tracked so we can ship the best first public version of Metro.
Attachment #8386555 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Went through the verification process using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-11-03-02-01-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-11-00-40-03-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/28.0b9/win32/en-US/

- Went through the original test case from comment #0 and made sure the original problem wasn't occurring
- Ensured that the "Open", "Save", "Cancel" and "X" buttons worked under the Navigation App Bar when the user initialized a download
- Ensured that once downloading, pressing the "X" button dismisses the "Download App Bar"
- Ensured "Show in Files" correctly switches the user to fxdesktop mode and opens the download directory without any issues
- Ensured "Open" correctly switches into fxdesktop and opens the file that has been downloaded
- Ensured that pressing the "X" once the download has completed doesn't dismiss the "Completed" download button
- Ensured that the download button doesn't disappear until a user either selects "Open" or "Show in Files"
- Ensured that you can still slide in the "Download App Bar" while on different tabs by pressing the download button under the Navigation App Bar
- Ensured that multiple app bar's are not being displayed
- Ensured that you can download multiple files without getting more app bar's than necessary
- Went through the above test cases in different variations of snapped view without any issues
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
You need to log in before you can comment on or make changes to this bug.