845408 - Do away with either downloads-button or downloads-indicator

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Because we currently support switching back to using the toolkit downloads window, we have 2 downloads buttons - one that opens the window, and another that opens the panel.

We do some sleight-of-hand with these buttons when doing things like customization, which can cause things like bug 844341.

Once we get rid of useToolkitUI support (bug 845403), we should get rid of the button that opens the download window, and just stick with the panel button. This would, theoretically, make bugs like bug 845403 go away.

Comment 1

[:Paolo Amadini]

"downloads-button" actually exists for startup performance reasons, since it
allows us to postpone loading the entire indicator overlay, with all its
associated XUL elements and scripts. It's actually unrelated to useToolkitUI.

Comment 2

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Oh. Hm. This was not my understanding of it. Thanks for clearing it up - removing dependency.

Comment 3

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

I wonder then if we can keep the same technique with postponed loading, but have the overlay inject the needed elements directly into downloads-button, instead of creating a new button?

How does that sound?

Comment 4

[:Paolo Amadini]

(In reply to Mike Conley (:mconley) from comment #3)
> I wonder then if we can keep the same technique with postponed loading, but
> have the overlay inject the needed elements directly into downloads-button,
> instead of creating a new button?

I guess there's nothing preventing us from doing that, except maybe that we need
to make things more complicated. In fact, those elements would also need to be
removed when entering toolbar customization mode, because Toolkit would otherwise
move those to the customization window.

Comment 5

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

(In reply to Paolo Amadini [:paolo] from comment #4)
> (In reply to Mike Conley (:mconley) from comment #3)
> > I wonder then if we can keep the same technique with postponed loading, but
> > have the overlay inject the needed elements directly into downloads-button,
> > instead of creating a new button?
> 
> I guess there's nothing preventing us from doing that, except maybe that we
> need
> to make things more complicated. In fact, those elements would also need to
> be
> removed when entering toolbar customization mode, because Toolkit would
> otherwise
> move those to the customization window.

I think reacting to customization is the better of two evils, as opposed to trying to find workarounds for our dual-button approach.

Comment 6

[Marco Bonardo [:mak]]

we should remeasure the perf issue, I'm not convinced it's due to the indicator.

Comment 7

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #4)
> (In reply to Mike Conley (:mconley) from comment #3)
> > I wonder then if we can keep the same technique with postponed loading, but
> > have the overlay inject the needed elements directly into downloads-button,
> > instead of creating a new button?
> 
> I guess there's nothing preventing us from doing that, except maybe that we
> need
> to make things more complicated. In fact, those elements would also need to
> be
> removed when entering toolbar customization mode, because Toolkit would
> otherwise
> move those to the customization window.

Dumb question: why is it a problem if those get moved to the customization window?

Comment 8

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to :Paolo Amadini from comment #4)
> > (In reply to Mike Conley (:mconley) from comment #3)
> > > I wonder then if we can keep the same technique with postponed loading, but
> > > have the overlay inject the needed elements directly into downloads-button,
> > > instead of creating a new button?
> > 
> > I guess there's nothing preventing us from doing that, except maybe that we
> > need
> > to make things more complicated. In fact, those elements would also need to
> > be
> > removed when entering toolbar customization mode, because Toolkit would
> > otherwise
> > move those to the customization window.
> 
> Dumb question: why is it a problem if those get moved to the customization
> window?

Because the elements are completely removed from the browser document, meaning that all our references to them are invalidated. When the Toolkit customization window is closed, the elements are destroyed and lost forever. Also, we don't have the styles to display the inner elements in the customization window.

I don't know if some of this applies to Australis customization also, or is only an issue for Toolkit customization.

Comment 9

[:Gijs (he/him)]

I'm poking at this because we're having quite a few subtle issues with the current state of affairs on UX. I have something which I hope is very close to working, I'm just ironing out the last few kinks in my manual testing before going over all the automated tests. (Famous last words, as it were)

Comment 10

[:Gijs (he/him)]

Attachment: unify download-indicator and download-button,

In fact, this passes both my manual checks (including dragging to/from the customize window), and the browser/base/* and browser/components/downloads/* mochitests. I've only done styling fixes for OS X, but those are pretty trivially ported. Not asking for review as I've not done that yet, but I'd love to have some feedback.

Comment 11

[:Paolo Amadini]

This doesn't work if I customize away the indicator, restart the browser, then restore the indicator. I think this was one of the most problematic cases when this was initially implemented, but now I'm testing on Linux without styles so it might be that it doesn't work because of that. I had a paused download while testing.

I hope the new approach here will work because the code is much simpler!

Comment 12

[:Paolo Amadini]

Ah, and I waited ten seconds after restart before restoring the indicator.

Comment 13

[:Gijs (he/him)]

Attachment: unify download-indicator and download-button,

I've fixed the issue that Paolo identified, I believe. Various things rely on _operational indicating whether the indicator is there, and it (a) needs to be reset more often than I thought (and before calling ensureTerminated), and (b) trying to load the overlay and setting that flag when we don't even have the placeholder (meaning the overlay won't apply to anything) is useless, so that can bail early and should never set _operational. I've tried this with various incantations of removing/re-adding, downloading inbetween, downloading with the button removed and then re-adding this, and this seems to do the trick. I've now also included style updates for Windows and Linux, which are basically just replacing '-indicator' with '-button' when it's at the end of the id, and adding '.indicator' if the style could (mistakenly) apply to the non-overlaid case. So, being bold, and asking for review. :-) (PS: this is a patch against m-c... the skin patches will definitely conflict on UX, but I've just described what I did so resolving that shouldn't be too tricky. Otherwise, I think this should Just Work on UX, but I'm happy to deal with fallout (which I expect to be style-only) if/when.

Comment 14

[Dão Gottwald [:dao]]

Why is this downloads button business a problem on UX but not on mozilla-central? If the customization code has issues, we shouldn't just make built-in widgets avoid them, because obviously they could affect add-ons as well, or hit us again in the future.

Comment 15

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #14)
> Why is this downloads button business a problem on UX but not on
> mozilla-central? If the customization code has issues, we shouldn't just
> make built-in widgets avoid them, because obviously they could affect
> add-ons as well, or hit us again in the future.

First off, UX does three things differently from m-c:

1) rearranging widgets happens not in a (modal?) dialog but in the main document
2) rearranging widgets affects all windows, not just the current one
3) the navbar can overflow

So the problems on UX (before this patch) are twofold, AIUI:

1) The skipintoolbarset item (indicator) having fun with our overflowing toolbar. This in and of itself isn't so much of a problem if we really didn't care about where the item is - but we do, it should be in the place of the placeholder (which *is* in the placements/currentset of the toolbar), and so it being moved about violates its constraints. The code here could implement detection for this kind of case, but really, getting rid of the magical extra button is simpler, lower maintenance, etc. etc.

2) The dynamic moving around of the indicator happening during customization mode. Maybe this couldn't happen as easily on m-c because customization is something of a modal dialog, but on UX it no longer is, and so code poking the toolbar's DOM without taking that into account is liable to break in interesting ways.

Now, these two problems could have been fixed in this code, by checking more extensively for customize mode and the overflowable toolbar. But the simpler solution is just to make the whole thing a couple of shades less magical.

There are separate issues with drag/drop of/onto skipintoolbarset items that we should try to address, but really, ideally widgets added by add-ons should not be using that attribute, but instead participate fully in customization (like the download button (placeholder) currently does, too).

If this bug gets fixed, that simplifies code, and (I think) solves most of the problems we currently have with this particular button. We can address the general skipintoolbarset vis-a-vis dnd issue separately; I was planning to file a followup bug after bug 916830 comment 2 but seem to have forgotten. I'll do that now.

Comment 16

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to Dão Gottwald [:dao] from comment #14)
> > Why is this downloads button business a problem on UX but not on
> > mozilla-central? If the customization code has issues, we shouldn't just
> > make built-in widgets avoid them, because obviously they could affect
> > add-ons as well, or hit us again in the future.
> 
> First off, UX does three things differently from m-c:
> 
> 1) rearranging widgets happens not in a (modal?) dialog but in the main
> document

It's not a modal dialog. Also, on Mac, the customization UI is loaded in an iframe in a panel. Pretty much the same thing as loading the UI in a tab, from a technical perspective.

Comment 17

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Comment on attachment 806833 [details] [diff] [review]
unify download-indicator and download-button,

This looks good, and the code looks like it's doing all of the right things. Kudos!

One thing I've noticed however, is that (at least on Windows 7), when I click on the downloads button on a fresh start-up, the URL bar and search input snap smaller and back, for like a single frame.

It's only 1 frame, but it's noticeable and jarring. I can reproduce the issue if I restart the browser, and then attempt to drag a link to the button.

Strangely, initiating a download on a page *does not* seem to cause this effect to occur, which makes me think this has something to do with the click / drop handlers, as opposed to the overlay being loaded.

So I think we should figure out what's happening there. But other than that, this works perfectly, and I can't wait for it to be landed. :)

Comment 18

[:Gijs (he/him)]

Attachment: unify download-indicator and download-button,

This seems to work locally, Mike, can you give this a whirl?

Comment 19

[:Gijs (he/him)]

Attachment: unify download-indicator and download-button,

So I moved the browser/base stuff into browser/base, but %included the skin bits into browser.css to keep blame. OTOH, this will likely be problematic for non-default themes that have put things in indicator.css, should there be any... Do we need to worry about that? Otherwise, AIUI this fixes the problem you were seeing (and is still a bit simpler than the previous incarnation of this patch).

Comment 20

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #19)
> Created attachment 811334 [details] [diff] [review]
> unify download-indicator and download-button,
> 
> So I moved the browser/base stuff into browser/base, but %included the skin
> bits into browser.css to keep blame. OTOH, this will likely be problematic
> for non-default themes that have put things in indicator.css, should there
> be any... Do we need to worry about that? Otherwise, AIUI this fixes the
> problem you were seeing (and is still a bit simpler than the previous
> incarnation of this patch).

Try push for ts_paint wariness:
https://tbpl.mozilla.org/?tree=Try&rev=2bf0502aec29

baseline:
https://tbpl.mozilla.org/?tree=Try&rev=4d57f65863f5

(both with PGO)

Comment 21

[:Gijs (he/him)]

> Try push for ts_paint wariness:
> https://tbpl.mozilla.org/?tree=Try&rev=2bf0502aec29
> 
> baseline:
> https://tbpl.mozilla.org/?tree=Try&rev=4d57f65863f5
> 
> (both with PGO)

Sigh. New try on OS X, with PGO off:

with patch:
https://tbpl.mozilla.org/?tree=Try&rev=5993a900f30a

baseline:
https://tbpl.mozilla.org/?tree=Try&rev=d93b9c3113fe

Comment 22

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #21)
> > Try push for ts_paint wariness:
> > https://tbpl.mozilla.org/?tree=Try&rev=2bf0502aec29
> > 
> > baseline:
> > https://tbpl.mozilla.org/?tree=Try&rev=4d57f65863f5
> > 
> > (both with PGO)
> 
> Sigh. New try on OS X, with PGO off:
> 
> with patch:
> https://tbpl.mozilla.org/?tree=Try&rev=5993a900f30a
> 
> baseline:
> https://tbpl.mozilla.org/?tree=Try&rev=d93b9c3113fe

Compare-talos for Linux and Windows:
http://compare-talos.mattn.ca/index.html?oldRevs=4d57f65863f5&newRev=2bf0502aec29&submit=true

Compare-talos for OS X:
http://compare-talos.mattn.ca/?oldRevs=d93b9c3113fe&newRev=5993a900f30a&submit=true

Here 10.8 seems a bit funny, but from the looks of the graph of m-c:
http://graphs.mozilla.org/graph.html#tests=[[83,1,24]]&sel=none&displayrange=7&datatype=running

this seems par for the course, although obviously it makes it hard to be 100% sure whether this actually regressed anything or not. Fortunately the results on Linux and Windows are fairly uniform and positive (even if I'm not sure how this change got us a tpaint/ts_paint *win* - it seems to be very slight anyway).

Assuming this gets through review, I think we should be OK. :-)

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 811334 [details] [diff] [review]
unify download-indicator and download-button,

>+#downloads-button.indicator > image.toolbarbutton-icon {
>+  display: none;
>+}

Make "indicator" an attribute instead of a class. As an attribute it's "private", because you usually qualify attribute selectors with tag or names or ids, so you don't need to worry about the same attribute name being used in other contexts. That's not true for classes, e.g. we could introduce a global "indicator" class and apply something to it that would break your use here.

Comment 24

[:Gijs (he/him)]

Attachment: unify download-indicator and download-button,

s/\.indicator/[indicator]/g

Comment 25

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Comment on attachment 811929 [details] [diff] [review]
unify download-indicator and download-button,

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

The code looks OK, but with this latest patch on Windows 7, on start-up, clicking the Downloads Button does not cause the panel to appear. I get "[12:29:08.107] Downloads button cannot be found @ chrome://browser/content/downloads/downloads.js:577" in the Browser Console...

::: browser/base/content/browser.css
@@ +688,4 @@
>    -moz-binding: url("chrome://browser/content/downloads/download.xml#download-toolbarbutton");
>  }
>  
> +/*** Visibility of indicator controls ***/

Let's make that "downloads indicator", to make it more specific.

Comment 26

[:Gijs (he/him)]

Attachment: unify download-indicator and download-button,

D'oh. Forgot to update the indicator check to check an attribute instead of a class. Here's a new and improved copy... :-)

Comment 27

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Comment on attachment 812120 [details] [diff] [review]
unify download-indicator and download-button,

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

Tested this, and it works just fine. :) I'm so glad that we're getting rid of the sleight-of-hand stuff.

Thanks Gijs!

::: browser/base/content/browser.css
@@ +688,4 @@
>    -moz-binding: url("chrome://browser/content/downloads/download.xml#download-toolbarbutton");
>  }
>  
> +/*** Visibility of indicator controls ***/

As before, I think "indicator controls" is to ambiguous. "Downloads indicator controls" might clear things up.

Comment 28

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/14457fb49b98

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/14457fb49b98

Comment 30

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/1d23e7dcbab2

For theme (and perhaps some add-on) authors: the "downloads-indicator" button has gone away. You should now use the "downloads-button" element. If you need to check that it has loaded its overlay, check for the "indicator" attribute on that button.

For theme authors: the chrome://browser/skin/downloads/indicator.css file is no longer referenced.

Comment 31

[:Gijs (he/him)]

I've updated https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/27 and asked for editorial review there to check that what I did was correct.

Comment 32

[:Gijs (he/him)]

Attachment: Fix label inheritance of button,

Mike de Boer noticed that labels are now missing on UX. A bit of debugging showed I should have copy-pasted properly from the relevant XBL binding, instead of whatever it is I did do.

Comment 33

[:Gijs (he/him)]

Comment on attachment 813161 [details] [diff] [review]
Fix label inheritance of button,

While this is probably necessary, it looks like it's not enough. I'll file a followup bug instead and look into it properly.

Comment 34

[Petruta Horea [:phorea], Desktop QA (back on May 31)]

Verified as fixed on latest Firefox 27 beta 1 (buildID: 20131209204824) using Win 7 64-bit, Win 8.1 64-bit, Mac 10.8.5, Ubuntu 13.04 32-bit.

Comment 35

[Aris]

Add-ons validator probably should not show the warning "The `#downloads-indicator` node was removed from the DOM.", if an add-on still supports Firefox 24-28 besides Fx29+, because #downloads-indicator` node is still present on Fx24 for example. This may confuse AMO editors.

> The `#downloads-indicator` node was removed from the DOM.
>Warning: The `#downloads-indicator` node was removed from the DOM. You should be able to use `#downloads-button` instead. See https://bugzilla.mozilla.org/show_bug.cgi?id=845408 for more information.

Comment 36

[:Paolo Amadini]

(In reply to Aris from comment #35)
> Add-ons validator probably should not show the warning "The
> `#downloads-indicator` node was removed from the DOM.", if an add-on still
> supports Firefox 24-28 besides Fx29+, because #downloads-indicator` node is
> still present on Fx24 for example. This may confuse AMO editors.

I agree that enough time has passed for this warning to have outlived its usefulness - an add-on that uses the old element without version checking wouldn't work in newer versions anyways.

Jorge, can the warning be removed?

Comment 37

[Jorge Villalobos [:jorgev] (he/him)]

Those warnings should show up in the compatibility category, which generally isn't very important to reviewers. I don't think it's a big deal to leave it there, though we should clean up some old warnings in the future.