Closed Bug 963143 Opened 6 years ago Closed 6 years ago

No download finished notification if download button is in the menu panel

Categories

(Firefox :: Downloads Panel, defect)

x86
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: mconley, Assigned: Gijs)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3-])

Attachments

(1 file)

STR:

1) Enter customize mode
2) Place the downloads button into the menu panel
3) Exit customize mode
4) Start and complete a download.

ER:

A download finished notification on the menu panel button to signal to the user that a download has completed.

AR:

No notification, at least on Windows and Linux. I do believe the notification works on OS X.
Not currently working on OS X, for sure (just tested).
Whiteboard: [Australis:P4] → [Australis:P3-]
So it seems like the DownloadsPanel code gets confused and ends up in kStateWaitingAnchor state and then never recovers. :|

Paolo, can you help here? It does seem to be somewhat intermittent...
Flags: needinfo?(paolo.mozmail)
I also easily reproduced the issue locally. I need to refresh my knowlegde of the front-end code, but I can definitely take a look in the next few days.
(In reply to :Gijs Kruitbosch from comment #2)
> So it seems like the DownloadsPanel code gets confused and ends up in
> kStateWaitingAnchor state and then never recovers. :|

Actually, I'm experiencing a different issue here. I moved the Downloads button to the menu, then restarted the browser.

Download notifications are shown correctly before the menu has been opened for the first time. After the menu has been opened once, notifications are not shown anymore.

The code that gets confused is here:

http://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/indicator.js#322-337

In particular, "DownloadsButton._placeholder" is a reference to the lazy-loaded "downloads-button" element, and is null before the menu is open. When it's not null, the line "if (anchor && isElementVisible(anchor.parentNode)) {" comes into effect, preventing the notification from being displayed.

I'm not familiar with that code, so I ask what is exactly the purpose of that check? The comment says "If the panel is open, don't do anything", but there is already a similar check above for DownloadsPanel.isPanelShowing. Is this check necessary for a different case?
Flags: needinfo?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #4)
> (In reply to :Gijs Kruitbosch from comment #2)
> > So it seems like the DownloadsPanel code gets confused and ends up in
> > kStateWaitingAnchor state and then never recovers. :|
> 
> Actually, I'm experiencing a different issue here. I moved the Downloads
> button to the menu, then restarted the browser.
> 
> Download notifications are shown correctly before the menu has been opened
> for the first time. After the menu has been opened once, notifications are
> not shown anymore.
> 
> The code that gets confused is here:
> 
> http://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/
> content/indicator.js#322-337
> 
> In particular, "DownloadsButton._placeholder" is a reference to the
> lazy-loaded "downloads-button" element, and is null before the menu is open.
> When it's not null, the line "if (anchor &&
> isElementVisible(anchor.parentNode)) {" comes into effect, preventing the
> notification from being displayed.
> 
> I'm not familiar with that code, so I ask what is exactly the purpose of
> that check? The comment says "If the panel is open, don't do anything", but
> there is already a similar check above for DownloadsPanel.isPanelShowing. Is
> this check necessary for a different case?

No, DownloadsPanel.isPanelShowing will check if the downloads panel is showing. This check is meant to check if the panel in which the button itself is situated (either the overflow panel or the main panel) is open, but clearly isElementVisible doesn't actually do what it says on the tin, because while the element is not visible, it reports it as being visible, probably because the boxObject's width and height aren't 0 even though somewhere in its parent chain an element (like the panel) is hidden.

So presumably we'd need to actually look for the panel that the node is in and check its state property.
This was fairly straightforward considering the comments, and it seems to fix things for me.
Attachment #8391252 - Flags: review?(mak77)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8391252 [details] [diff] [review]
downloads animation no longer showing when button is in panel,

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

::: browser/components/downloads/content/indicator.js
@@ +303,5 @@
> +   *        the node whose panel we're interested in.
> +   */
> +  _isAncestorPanelOpen: function DIV_isAncestorPanelOpen(aNode)
> +  {
> +    while (aNode && aNode.parentNode && aNode.localName != "panel") {

don't null-check parentNode here...

@@ +306,5 @@
> +  {
> +    while (aNode && aNode.parentNode && aNode.localName != "panel") {
> +      aNode = aNode.parentNode;
> +    }
> +    return aNode && aNode.localName == "panel" && aNode.state == "open";

... so you can avoid checking localName here, we'd always exit the loop with a null or with a panel

@@ +333,5 @@
>      }
>  
>      let anchor = DownloadsButton._placeholder;
>      let widgetGroup = CustomizableUI.getWidget("downloads-button");
>      let widgetInWindow = widgetGroup.forWindow(window);

while here, may we avoid this crazy widgetInWindow naming and just name it downloadsWidget or widget?

@@ +336,5 @@
>      let widgetGroup = CustomizableUI.getWidget("downloads-button");
>      let widgetInWindow = widgetGroup.forWindow(window);
>      if (widgetInWindow.overflowed || widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL) {
> +      if (anchor && this._isAncestorPanelOpen(anchor)) {
> +        // If the containing panel is open, don't do anything:

Please complete the comment explaining the issue, as you did on IRC ("since the panel overflows the animation"), otherwise this reads wrong.
We should have a bug filed to investigate if there's a way to show animations in the panel, and would be nice here to have that bug number reported in this comment.

nit: s/:/./
Attachment #8391252 - Flags: review?(mak77) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/3ee3e227a0c0
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
you fixed only half of the comments :/
(In reply to Marco Bonardo [:mak] from comment #9)
> you fixed only half of the comments :/

Ugh. When I saw the bugmail, I only noticed the top bit. Sorry. I will try to followup late tonight from the airport.
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 984023
https://hg.mozilla.org/mozilla-central/rev/3ee3e227a0c0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Target Milestone: --- → Firefox 30
remote:   https://hg.mozilla.org/integration/fx-team/rev/20a6cc189fdd
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8391252 [details] [diff] [review]
downloads animation no longer showing when button is in panel,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 916256
User impact if declined: no downloads animation is shown if the download button is in the menu panel
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8391252 - Flags: approval-mozilla-aurora?
Attachment #8391252 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: cornel.ionce
I reproduced this issue on the Nightly from January 23rd. It reproduced quite easy and fast, but it was intermittent.

I performed several downloads on the latest Beta and Aurora (Win 7 x86, Ubuntu 13.04 x86, Mac OS X 10.9.2) and didn't see the issue anymore.

(Given it was intermittent to start with, the verification of this bug is not 100% reliable.)
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce → ioana.budnar
You need to log in before you can comment on or make changes to this bug.