365634 - restructure <download> content for when the progress meter is visible to better use the UI space

Status: RESOLVED INCOMPLETE

(Toolkit :: Downloads API, enhancement)

Description

[Declan Naughton]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a2pre) Gecko/20070102 Minefield/3.0a2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a2pre) Gecko/20070102 Minefield/3.0a2pre

Currently the progress bar doesn't fill as much of the window as it can, due to it's structure.

The download-downloading and download-paused bindings in chrome://mozapps/content/downloads/download.xml could be better restructured to use the UI space more efficiently.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Declan Naughton]

Attachment: Do the restructuring! v1

it also does some general tidyup.. replacing vars with consts and removing excess trailing whitespace as I went along.

Comment 2

[Declan Naughton]

Attachment: Screenshot of restructured elements with the patch.

Note that the Cancel buttons don't line up with the Open/Remove/Retry ones. I don't know how to fix that. It seems to be the basis of our current structure, hopefully we can have it both ways.

Comment 3

[Declan Naughton]

Comment on attachment 250155 [details] [diff] [review]
Do the restructuring! v1

Slightly inconsistent, but tested and working (on GNU/Linux, but I'd expect it to work on all platforms). See screenshot.

Comment 4

[Declan Naughton]

Attachment: as above + missing status on pause state

same patch as above, just with the missing status beside the resume button in the paused state. whoops.

Comment 5

[Declan Naughton]

Attachment: Restructures the relevant download manager bindings so that the progress bar fills to the end of the window

(whenever I mentioned inconsistent behaviour before.. that was in error. Cancel links were never lined up with remove/retry links.)

This patch also removes a bit of excess whitespace, and it #ifdef 0's out unused code (the icon animation. To my knowledge, nobody uses it. If it works at all.).

I added style="border: 0" to labels of class text-link where the link is horizontally next to another non-text-link label (whether a spacer separates them or not), because text-links appear to have a 1px border where labels do not, and would therefore otherwise not match up horizontally with the line. I'm not sure if this is ideal - if it hurts themability.

Tested on GNU/Linux - looks the part, but for OS X and Windows, and for different themes, I wouldn't be so sure things will match up due to the border thing I mentioned....

(this theme stuff I'm not more than only a little familiar with. Feel free to fix & pick up the patch if you like.)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Sorry that it took so long for me to get to this, I really shouldn't have left it in my review queue for so long without commenting. Subsequent iterations should be quicker, I promise.

(In reply to comment #1)
> it also does some general tidyup.. replacing vars with consts and removing
> excess trailing whitespace as I went along.

This "extra cleanup" makes the patch harder to review. I think the s/var/const/ changes are gratuitous, and if you're making many whitespace changes it's a good idea to post a -w patch for review.

It seems like you're making a behavioral change to the "removable" getter, could you explain it? Or perhaps move it over to another bug if it's a bug fix.

I'm not sure if this still applies, hopefully my review delay has not put a damper on your interest in fixing this bug :/.

Comment 7

[Shawn Wilsher :sdwilsh]

I think this has been fixed for Firefox 3, no?

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:mak, could you have a look please?
For more information, please visit auto_nag documentation.