Closed Bug 1117145 Opened 6 years ago Closed 5 years ago

Unify the "download.xml" binding between the Panel and the Library

Categories

(Firefox :: Downloads Panel, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 47
Iteration:
47.3 - Mar 7
Tracking Status
firefox47 --- fixed

People

(Reporter: Paolo, Assigned: Paolo, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(5 files)

The XBL binding used to display download items is different for the Downloads Panel and the Downloads View, but actually quite similar in structure.

Unfortunately, unifying the two bindings is not that easy because the exact structure is used in theme CSS, which requires lots of testing.

But now we need to modify the XUL structure of the element anyways, in order to implement the new state for downloads that can be unblocked. This new state should work the same way from all views, so it may be worth looking into using the same base CSS first.

This work can probably start on top of bug 1117141.

At the same time or maybe just after the unification, it may be worth looking into removing the "state" attribute completely, and simplify the controlling CSS, that is currently difficult to understand.
Note that having a build on more than one platform (Windows, Linux, or Mac) is recommended to test the theme changes.
Mentor: paolo.mozmail
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [diamond][lang=js]
Hey Paolo, I am interested.(Actually came from bug 936973) I was wondering if this is on high priority/tight schedule? Moreover where should one start?
Flags: needinfo?(paolo.mozmail)
(In reply to Abhishek Bhattacharya [:babhishek21] from comment #2)
> Hey Paolo, I am interested.(Actually came from bug 936973) I was wondering
> if this is on high priority/tight schedule?

Thanks for your interest!

With regard to priority, we think this bug is important and can lead to visible improvements to the Downloads Panel and Library window, but the visual design here hasn't been prioritized as one of the immediate responsibilities of the team. This means it is a good opportunity, as it doesn't have a tight schedule, but keep in mind that turnaround time might be slower than usual as I devote attention to higher priorities.

> Moreover where should one start?

Take a look at "browser/components/downloads/content/download.xml". There are two bindings there, and in the end we want to get rid of the "full-ui" one.

We can work in steps. The two bindings have a slightly different structure from each other, for example the full-ui one doesn't have a <xul:stack> around the buttons. The first step could be to update the full-ui binding to use a stack as well, and update the relevant CSS so that the rules still match the elements.
Flags: needinfo?(paolo.mozmail)
Flags: qe-verify?
Whiteboard: [diamond][lang=js] → [diamond][lang=js] [fxprivacy]
Flags: qe-verify? → qe-verify-
Priority: -- → P3
Priority: P3 → P2
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Iteration: --- → 47.1 - Feb 8
Priority: P2 → P1
Depends on: 1244473
Iteration: 47.1 - Feb 8 → ---
Priority: P1 → P2
Whiteboard: [diamond][lang=js] [fxprivacy] → [diamond][lang=js] [fxprivacy] [blocked]
Whiteboard: [diamond][lang=js] [fxprivacy] [blocked] → [diamond][lang=js] [fxprivacy]
Actually, I'm working on this right now, so clearing the whiteboard tags.
Priority: P2 → P1
Whiteboard: [diamond][lang=js] [fxprivacy] → [fxprivacy]
MozReview currently doesn't show the contents of the removed files, but the changeset above removed duplication by having one CSS file instead of three with repeated rules to control the visibility of the elements inside download items.

This first pass also unifies how the buttons are handled, using "visibility: hidden" inside a stack element instead of "display: none" in the Library.
Iteration: --- → 47.1 - Feb 8
Iteration: 47.1 - Feb 8 → 47.2 - Feb 22
Comment on attachment 8716296 [details]
MozReview Request: Bug 1117145 - Part 1 - Unify the functional CSS files for the Downloads Panel and the Library. r=jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33779/diff/1-2/
Attachment #8716296 - Attachment description: MozReview Request: Bug 1117145 - Part 1 - Unify the functional CSS files for the Downloads Panel and the Library. → MozReview Request: Bug 1117145 - Part 1 - Unify the functional CSS files for the Downloads Panel and the Library. r=jaws
Attachment #8716296 - Flags: review?(jaws)
Marco, let me know if you want to provide feedback on the patches. I've requested review from Jared.
Flags: needinfo?(mak77)
I'll take a look when I can, don't block on me since I have far too many requests to handle already.
Flags: needinfo?(mak77)
Comment on attachment 8719293 [details]
MozReview Request: Bug 1117145 - Part 5 - Share the implementation of some of the download item commands. r=jaws

https://reviewboard.mozilla.org/r/34927/#review31861
Attachment #8719293 - Flags: review?(jaws) → review+
Comment on attachment 8719292 [details]
MozReview Request: Bug 1117145 - Part 4 - Unify how commands are handled in the Downloads Panel and the Library. r=jaws

https://reviewboard.mozilla.org/r/34925/#review31583

::: browser/components/downloads/content/allDownloadsViewOverlay.js:354
(Diff revision 1)
> -      case "downloadsCmd_open": {
> +      this[aCommand](this);

I don't see any commands that use the `this` argument, so it looks like this can be simplified to just `this[aCommand]()`.

Did you intend to write `this[aCommand].bind(this)();` ?

::: browser/components/downloads/content/allDownloadsViewOverlay.js:1467
(Diff revision 1)
> +      if (name.startsWith("cmd_") || name.startsWith("downloadsCmd_")) {

Lines 1185 and 1186 do this similar check, along with a couple places in downloads.js. Can you factor this out to a `isValidCommand()` function?

::: browser/components/downloads/content/downloads.js:1095
(Diff revision 1)
> -      this.commands[aCommand].apply(this);
> +      this[aCommand](this);

What is the point of passing `this` as the first argument here?
Attachment #8719292 - Flags: review?(jaws) → review+
Attachment #8719291 - Flags: review?(jaws) → review+
Comment on attachment 8719291 [details]
MozReview Request: Bug 1117145 - Part 3 - Share the download item binding between the Panel and the Library. r=jaws

https://reviewboard.mozilla.org/r/34923/#review31863
Attachment #8719290 - Flags: review?(jaws) → review+
Comment on attachment 8719290 [details]
MozReview Request: Bug 1117145 - Part 2 - Unify DownloadsViewItem and DownloadsViewItemController. r=jaws

https://reviewboard.mozilla.org/r/34921/#review31865
Comment on attachment 8716296 [details]
MozReview Request: Bug 1117145 - Part 1 - Unify the functional CSS files for the Downloads Panel and the Library. r=jaws

https://reviewboard.mozilla.org/r/33779/#review31563

::: browser/components/downloads/content/downloads.css:17
(Diff revision 2)
> +{
> +  display: none;
> +}
> +
> +#downloadsSummary:not([inprogress]) > vbox > #downloadsSummaryProgress,
> +#downloadsSummary:not([inprogress]) > vbox > #downloadsSummaryDetails,
> +#downloadsFooter[showingsummary] > #downloadsHistory,
> +#downloadsFooter:not([showingsummary]) > #downloadsSummary
> +{
> +  display: none;
> +}

nit, place the opening bracket on the same line as the last selector
Attachment #8716296 - Flags: review?(jaws) → review+
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7
Depends on: 1279329
Depends on: 1280799
Depends on: 1276973
You need to log in before you can comment on or make changes to this bug.