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

RESOLVED FIXED in Firefox 47

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Paolo, Assigned: Paolo, Mentored)

Tracking

(Blocks 1 bug)

Trunk
Firefox 47
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [fxprivacy])

Attachments

(5 attachments)

Assignee

Description

4 years ago
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.
Assignee

Comment 1

4 years ago
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)
Assignee

Comment 3

4 years ago
(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]
Assignee

Updated

4 years ago
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
Assignee

Updated

3 years ago
Depends on: 1244473
Iteration: 47.1 - Feb 8 → ---
Priority: P1 → P2
Whiteboard: [diamond][lang=js] [fxprivacy] → [diamond][lang=js] [fxprivacy] [blocked]
Assignee

Updated

3 years ago
Whiteboard: [diamond][lang=js] [fxprivacy] [blocked] → [diamond][lang=js] [fxprivacy]
Assignee

Comment 4

3 years ago
Actually, I'm working on this right now, so clearing the whiteboard tags.
Priority: P2 → P1
Whiteboard: [diamond][lang=js] [fxprivacy] → [fxprivacy]
Assignee

Comment 6

3 years ago
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
Assignee

Comment 7

3 years ago
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)
Assignee

Comment 12

3 years ago
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.
Assignee

Updated

3 years ago
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+
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
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

Updated

3 years ago
Depends on: 1279329

Updated

3 years ago
Depends on: 1276973
You need to log in before you can comment on or make changes to this bug.