803546 - Previewing background theme causes Downloads button to jump around.

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1)  But the Downloads button anywhere but in the right-most position on the navigation toolbar.
2)  Go to https://www.getpersonas.com/
3)  Keep your eyes on the Downloads button
4)  Hover the mouse over a Featured theme to temporarily apply it.

What happens?

The Downloads button moves to the end of the toolbar, and stays there.

Clicking on the moved button causes it to jump back.

What's expected?

The button should not move when a background theme is applied.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

This only seems to affect Windows 7, and I can recreate it simply by attaching a new binding to the navigation toolbar - so I don't think it's background them specific...

Comment 2

[Marco Bonardo [:mak]]

could be related to bug 640158?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Marco Bonardo [:mak] from comment #2)
> could be related to bug 640158?

Hm, no, I don't think so - we're not re-loading any overlays when we apply a background theme.

I think I'm narrowing in on the issue - it looks like when we insert the indicator in place of the downloads-button, the toolbar's currentSet Javascript accessor is being updated - but the node's attribute is not. Very strange.

And this does nothing to explain why I'm only seeing the issue on my Windows 7 machine.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

At the very least, I've got a workaround that fixes this - but I'm going to dig a little deeper and find out why toolkit is not updating that toolbar's currentset attribute.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Ah, apparently this is a known limitation:

https://developer.mozilla.org/en-US/docs/XUL/Property/currentSet

Comment 6

[Marco Bonardo [:mak]]

(In reply to Mike Conley (:mconley) from comment #3)
> (In reply to Marco Bonardo [:mak] from comment #2)
> > could be related to bug 640158?
> 
> Hm, no, I don't think so - we're not re-loading any overlays when we apply a
> background theme.

But we do when we first replace the button with the indicator. Btw may be unrelated, just wanted to point out a known issue that could be related, even better if it's not.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Ok, here are my findings:

1) The customization dialog takes care of writing to the currentset attribute when a toolbar's currentSet changes.

2) For whatever reason, on Windows 7, when a new binding is attached to the navigation bar, it reinitializes, and resets its buttons to the "currentset". This causes the flicker if currentSet and currentset are out of sync.

3) We do some trickery to slip the downloads-indicator next to the downloads-button. However, we do not update the currentset attribute when we do so, which causes #2 to happen.

I have a patch coming that fixes this issue for me.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 676606 [details] [diff] [review]
Patch v1

Are there any edge-cases I'm not considering? Any situations where the parent node of indicator is not something we want to set currentset on?

Comment 10

[:Paolo Amadini]

The fact that currentSet is updated dynamically based on the actual toolbar
content is something I didn't consider. I don't think the dynamically inserted
indicator should be included in the currentSet, since it cannot be found when
the browser starts up again. I just found an attribute named "skipintoolbarset",
maybe setting it on the indicator element is enough to solve the issue?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

No luck with skipintoolbarset.

However, since skipintoolbarset makes the currentSet and currentset attributes not change, and the problem persists despite this, I think my original findings are a red-herring.

Digging deeper into toolbar.xml...

Comment 12

[Mike Conley (:mconley) (:⚙️)]

So, just to record a conversation Marco and I just had in IRC:

The reason this is only happening in Windows 7 is because the Windows Aero theme applies a toolbar-drag binding to the navigation toolbar when a background theme is applied.

One solution is to simply not apply the binding when a background theme is applied, but that still causes button switch-y behaviour if an add-on attaches a binding.

What we're considering is merging both the downloads-button and downloads-indicator into a single toolbarbutton, that switches between the two "states".

Paolo - do you have any objections or concerns with that attack plan?

Comment 13

[Marco Bonardo [:mak]]

well more than a single toolbarbutton, a toolbaritem container where we'll put the button or the indicator (see bookmarks-menu-button-container for example)

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

WIP patch, which wraps up both the placeholder and the indicator within their own container toolbaritem.

If we keep this structure, with the same IDs, it will necessitate adding another migration to nsBrowserGlue.js to remote downloads-button, and insert downloads-button-container. Is that OK? Or is it preferable to switch the IDs of the placeholder, and name the toolbaritem downloads-button?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Also, with the WIP patch, I can confirm that attaching a binding no longer causes the downloads buttons to flip around. Woo!

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 2

I've changed my mind here - I don't want to get the overlay to insert the indicator into the downloads-button-container automatically. Too many edge cases where the button might not exist at startup, etc. I think the old solution of putting it into a popup is sufficient.

But my original question about migrations still applies.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 677198 [details] [diff] [review]
WIP Patch 2

I don't think it's ok to reuse downloads-button id for another kind of item, cause code and add-ons may make assumptions on it being a button, so better do what you did here.
Using a migrationUI step is fine, it's the common way to handle these migrations, we'll also need a new patch to disable the panel in Aurora that removes the container from the currentset (So far we removed downloads-button) in case we have to disable the panel in aurora for 19 (a bug apart should be filed for this).
You have to manage customization too since the user drags the button but we actually position the container.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

(In reply to Marco Bonardo [:mak] from comment #17)
> Comment on attachment 677198 [details] [diff] [review]
> WIP Patch 2
> 
> I don't think it's ok to reuse downloads-button id for another kind of item,
> cause code and add-ons may make assumptions on it being a button, so better
> do what you did here.
> Using a migrationUI step is fine, it's the common way to handle these
> migrations, we'll also need a new patch to disable the panel in Aurora that
> removes the container from the currentset (So far we removed
> downloads-button) in case we have to disable the panel in aurora for 19 (a
> bug apart should be filed for this).

Cool, can do.

> You have to manage customization too since the user drags the button but we
> actually position the container.

Are you sure? With this latest patch, if I customize and drag the downloads-button around, the container seems to come with. I can't seem to get into a case where I drag the button outside of the container. Do you have STR for this behaviour?

Comment 19

[Marco Bonardo [:mak]]

(In reply to Mike Conley (:mconley) from comment #18)
> Are you sure? With this latest patch, if I customize and drag the
> downloads-button around, the container seems to come with.

Ah, it's possible the customize code just drags whatever item is a direct descendant of the toolbar, so this magically works. I don't recall off-hand but would make sense, you are actually dragging the toolbaritem and all of its descendants.
So just test what happens if the container is initially in the palette, and gets moved later to its position, and viceversa, that is the most common case where things start to break.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

(In reply to Marco Bonardo [:mak] (Away 2 Nov) from comment #19)
> So just test what happens if the container is initially in the palette, and
> gets moved later to its position, and viceversa, that is the most common
> case where things start to break.

Seems to work fine - especially since I'm keeping the indicator initially in the popupset before we insert it after the downloads-button...that seems to take care of the cases where the button does not exist in the browser on startup.

I should note, however, that while testing this, when the downloads-button is not in the palette, the old Downloads window is opening when a download begins. Is this a regression? Did we not want to anchor the panel somewhere else for this? Or am I misremembering?

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Clearing needinfo - because I'm 99% positive that anchoring the panel to the tabs toolbar when the button is not visible is the desired behaviour. Dealing with that in bug 807708.

Comment 23

[:Paolo Amadini]

Comment on attachment 677446 [details] [diff] [review]
Patch v1

One issue with placing "downloads-button" into a new item named
"downloads-button-container", that takes its place, is that it affects the
installations where the Downloads Panel feature is disabled, more than those
where it has been enabled.

Changing the ID of the outer element means that we should run migration code
in all cases, finding out on which toolbar downloads-button is located, and
replacing it with downloads-button-container so that everything continues to
work as before on Aurora where the panel is not enabled.

I'm also concerned that the restore code might move "downloads-button" outside
of its container if it was in the currentSet of any toolbar, when processing
customization restore on startup. Though, I don't actually know if the item
with the given ID must be a direct descendant of a toolbar or the palette to
trigger that behavior.

If we keep the outer item's ID, then we don't have these issues. As a
compromise to make things easier now, I think we could keep the outer item's
ID to be "downloads-button" for now, and file a bug blocking the panel's
release, to switch the ID to be "downloads-button-container", at the same
time as we run the migration code that moves the button from wherever the
user placed it to the final place near the search bar and home button.
What do you think?

In any case, the customization code should also be reworked. If the indicator
is inside another toolbaritem while customizing, when the toolbaritem is
moved to the palette wuindow, all its children are removed from the browser
window as well. So the indicator pointed to by the lazy getter is lost forever,
even if the element returns later:

http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/indicator.js#539

To solve, we might just move the indicator to a safe place while customizing.

Comment 24

[:Paolo Amadini]

(In reply to Mike Conley (:mconley) from comment #22)
> Clearing needinfo - because I'm 99% positive that anchoring the panel to the
> tabs toolbar when the button is not visible is the desired behaviour.

If the design is still valid, when a new download is started and the indicator
is not visible, we should show the primary download management interface in the
Library window (or the old Downloads window for now).

The panel should only ever be shown if the indicator is visible. If the indicator
is hidden or has been removed from the toolbars, we should never show the panel:

https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager

Comment 25

[Mike Conley (:mconley) (:⚙️)]

(In reply to Paolo Amadini [:paolo] from comment #24)
> (In reply to Mike Conley (:mconley) from comment #22)
> > Clearing needinfo - because I'm 99% positive that anchoring the panel to the
> > tabs toolbar when the button is not visible is the desired behaviour.
> 
> If the design is still valid, when a new download is started and the
> indicator
> is not visible, we should show the primary download management interface in
> the
> Library window (or the old Downloads window for now).
> 
> The panel should only ever be shown if the indicator is visible. If the
> indicator
> is hidden or has been removed from the toolbars, we should never show the
> panel:
> 
> https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager

Oh my - whoops. I had made an assumption based on this code:

http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#328

What is the case where we'd want the panel anchored to the tabs toolbar?

Comment 26

[:Paolo Amadini]

(In reply to Mike Conley (:mconley) from comment #25)
> What is the case where we'd want the panel anchored to the tabs toolbar?

Basically, never. That code is just a safety net I left over from previous
iterations, can also be replaced with a simple Cu.reportError as IIRC we
discussed on IRC :-)

Comment 27

[Mike Conley (:mconley) (:⚙️)]

(In reply to Paolo Amadini [:paolo] from comment #26)
> (In reply to Mike Conley (:mconley) from comment #25)
> > What is the case where we'd want the panel anchored to the tabs toolbar?
> 
> Basically, never. That code is just a safety net I left over from previous
> iterations, can also be replaced with a simple Cu.reportError as IIRC we
> discussed on IRC :-)

Cool - filed bug 808054.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Ok, so now the outer container is called "downloads-button". That was surprisingly easy, since I really just had to change the ID of the node that DownloadsButton._placeHolder queries for. No ill effects, it seems.

I've also wrapped the indicator node in a "safe place" node in the indicator overlay (I didn't give the popupset an ID, since this would prevent it from being inserted into the document).

The safe place is where we're stashing the indicator when we're customizing, or when the downloads-button does not exist on any toolbars.

Comment 29

[Marco Bonardo [:mak]]

Comment on attachment 677862 [details] [diff] [review]
Patch v2

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

I'm still doing some investigation on this, so I'm not ready for a review yet. Just some thoughts so far...

::: browser/base/content/browser.xul
@@ +941,5 @@
>             during the customization of the toolbar, in the palette, and before
>             the Downloads Indicator overlay is loaded. -->
> +
> +      <toolbaritem id="downloads-button" class="chromeclass-toolbar-additional"
> +                     title="&downloads.label;">

I'm surprised the various css rules with -moz-image-region on #downloads-button still work properly, they now apply to the toolbaritem than the button, so what happens when we hide the button and put the indicator into the container, won't it get the rule, when it actually doesn't pertain to it?

Plus at least downloads-button here should be changed or we'd regress again the styling: http://mxr.mozilla.org/mozilla-central/source/browser/themes/browserShared.inc#3

I still fear this "use the same id for different widget" thing and the subtle bugs (like the one above) it will introduce across custom themes. If we couldn't make it right at first I'm not optimist about third parties.

Paolo, could you better explain which kind of issues we may be hitting if we do a migration step and migrate all of the users to the container, regardless the feature being enabled?
If the most problematic issue is with code that may move downloads-button out of the container, I may even be fine renaming both the container and the button and just lose the current customization position of downloads-button, regardless that is likely what will happen in future. Full breakage is better than hidden breakage.

::: browser/components/downloads/content/indicatorOverlay.xul
@@ +23,5 @@
> +         back into the palette, we hide the indicator here. If they then move
> +         the button back into a toolbar, we programmatically put this indicator
> +         back inside it when it is needed. This keeps us from needing to juggle
> +         references to DOM nodes. -->
> +    <toolbaritem id="downloads-indicator-safe-place">

why not just giving an id "downloads-indicator-parking" to the popupset and park the indicator back to its initial position inside it? The additional toolbaritem doesn't look mandatory

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Updated STR:

STR:

1)  Put the Downloads button anywhere but in the right-most position on the navigation toolbar.
2)  Click on the Downloads button to expose the panel, and then close the panel.
3)  Go to https://www.getpersonas.com/
4)  Keep your eyes on the Downloads button
5)  Hover the mouse over a Featured theme to temporarily apply it.

What happens?

The Downloads button moves to the end of the toolbar, and stays there.

Clicking on the moved button causes it to jump back.

What's expected?

The button should not move when a background theme is applied.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

(In reply to Marco Bonardo [:mak] from comment #29)
> why not just giving an id "downloads-indicator-parking" to the popupset and
> park the indicator back to its initial position inside it? The additional
> toolbaritem doesn't look mandatory

If I add an ID to the popupset, the node does not get added to the document.

I believe this is how XUL overlays are designed to work - if I provide an ID to one of the root nodes in the overlay, Gecko will attempt to append the children of that node to a similar node with the same ID in the overlayed document. Since no such popupelement with id="downloads-indicator-parking" or "downloads-indicator-safe-place" exists, the node doesn't get added.

Nodes without IDs are added to the document outright.

Or perhaps I am misunderstanding how overlays work?

Comment 32

[Marco Bonardo [:mak]]

ah, overlay madness :(

Comment 33

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini [:paolo] from comment #23)
> So the indicator pointed to by the lazy getter is lost
> forever,
> even if the element returns later:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/
> content/indicator.js#539
> 
> To solve, we might just move the indicator to a safe place while customizing.

Couldn't we just change the getter to check this._indicator (rather than replacing the getter with the value) and delete _indicator on customizeDone so the getter fetches the new element? I don't like much the added complexity due to "parking" the indicator.
Also, please see the question in comment 29 about renaming.

Comment 34

[Marco Bonardo [:mak]]

aww, I now noticed the indicator getter is in the view... so what's the downside of not caching it at all? just getElementById?

Comment 35

[Marco Bonardo [:mak]]

Attachment: hack

so, this is an alternative hack based on the fact customization basically ignores anything that is not a toolbarXXX element, so I wrap the overlay indicator in an hbox and move it to the proper position rather than moving the button.
It seems to fix this bug and is cleaner, though I didn't put much insight into verifying edge cases (for example if downloads is after back-forward and I move it into the palette, the overlay stays there breaking the merge of back and the urlbar, so at customizeDone it's not enough to just collapse the indicator, we must move it somewhere else as well, to the palette maybe?).
May be good for some brainstorming/experiment.

Comment 36

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #29)
> I still fear this "use the same id for different widget" thing and the
> subtle bugs (like the one above) it will introduce across custom themes. If
> we couldn't make it right at first I'm not optimist about third parties.
> 
> Paolo, could you better explain which kind of issues we may be hitting if we
> do a migration step and migrate all of the users to the container,
> regardless the feature being enabled?
>
> If the most problematic issue is with code that may move downloads-button
> out of the container

That was the concern (can be verified with a bit of testing), and mainly the
need for an additional migration step that is much more complex than the ones
we have today. But you have a good point about custom themes expecting the
element to be a toolbarbutton.

(In reply to Marco Bonardo [:mak] from comment #33)
> Couldn't we just change the getter [...]
> so the getter fetches the new element?

That's a valid alternative we also discussed, and came to the conclusion that
there is no strong reason to prefer one way or the other. With this approach
we should just verify that the DownloadsIndicatorView works correctly or is
uninitialized at the right time if at some point it doesn't find the elements,
this just requires some more testing than the other alternative.

(In reply to Marco Bonardo [:mak] from comment #35)
> so, this is an alternative hack based on the fact customization basically
> ignores anything that is not a toolbarXXX element

How is this different from the "skipintoolbarset" attribute (comment 10 and
comment 11)? If the two approaches behave differently, maybe this can be fixed
at the platform level and we can just use "skipintoolbarset" on the indicator.

The attribute was introduced in bug 585946, which also mentions a re-binding
issue, not sure if it's related.

Comment 37

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini [:paolo] from comment #36)
> How is this different from the "skipintoolbarset" attribute (comment 10 and
> comment 11)? If the two approaches behave differently, maybe this can be
> fixed
> at the platform level and we can just use "skipintoolbarset" on the
> indicator.

that would be nice to do, we should investigate why it differs.

Comment 38

[Marco Bonardo [:mak]]

btw, as a side note, even with the skipintoolbarset approach, we will still need to park the indicator somewhere when the button is removed, since the indicator, even if collapsed, would break magic merges like back-forward+urlbar.

Comment 39

[Marco Bonardo [:mak]]

fwiw, the skipintoolbarset change fixes the bug for me... Mike could you please retest it?

Comment 40

[Marco Bonardo [:mak]]

Attachment: skipintoolbarset patch

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 678662 [details] [diff] [review]
skipintoolbarset patch

Wow - this works really well. I'm surprised, since I'm certain I tried the "skipintoolbarset" solution last week, with no success.

I'm quite happy with this solution - I like how simple it is.

Thanks Marco!

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 678495 [details] [diff] [review]
hack

This essentially does the same thing as your skipintoolbarset patch, but takes advantage of toolbar.xml's ignorance of hbox's. I think I prefer the explicit usage of skipintoolbarset.

Comment 43

[Marco Bonardo [:mak]]

Comment on attachment 677862 [details] [diff] [review]
Patch v2

ok so, I suppose we all agree for the skipintoolbarset change

Comment 44

[Marco Bonardo [:mak]]

Comment on attachment 678662 [details] [diff] [review]
skipintoolbarset patch

let's flip the flags.

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 678662 [details] [diff] [review]
skipintoolbarset patch

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

This looks good - but I think the comment needs to be updated.

Thanks Marco!

::: browser/components/downloads/content/indicator.js
@@ +156,4 @@
>      if (!placeholder) {
>        // The placeholder has been removed from the browser window.
>        indicator.collapsed = true;
> +      // Move the indicator to a safe position, since staying on the toolbar

I think this comment needs to be updated, since the indicator could still be on the toolbar (in the event that it was adjacent to the downloads-button).

This simply places the indicator at the end of the toolbar, collapsed.

Comment 46

[Marco Bonardo [:mak]]

Assigning to myself just cause of the patch ownership, this was basically a team effort.

Comment 47

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52399a038bbe

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52399a038bbe

Comment 49

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121112030753

The Downloads button still jumps for me on the latest Nightly when applying a background theme.

Steps to reproduce:
1. Launch Firefox with a new profile
2. Navigate a to https://addons.mozilla.org/en-US/firefox/personas/
3. Click on one of the personas -> select the button Add to Firefox
4. After the first background persona is applied, choose a different persona and select the button Add to Firefox.

Actual results: After selecting the second persona the Downloads button jumps to the right-most position on the navigation toolbar. After clicking on it the button jumps back to the original position.
Please see the screencast: http://screencast.com/t/LRONYRgQTE

Would you prefer me to Reopen this bug or file a new one?

Comment 50

[Marco Bonardo [:mak]]

I can reproduce following your screencast. Steps are different though, so please file a new bug and make it block ReleaseDownloadPane

Comment 51

[Marco Bonardo [:mak]]

so basically this bug happened when previewing a background theme, while comment 49 happens when applying them.

Comment 52

[Simona Badau, Desktop QA]

(In reply to Marco Bonardo [:mak] from comment #50)
> I can reproduce following your screencast. Steps are different though, so
> please file a new bug and make it block ReleaseDownloadPane

Thanks Marco, filed Bug 811263.

Comment 53

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121112030753

Verified using the latest Nightly on Windows 7 that the Downloads button does not jump around when previewing background themes.

Setting the status of this bug to Verified Fixed.