Avoid redundant elements in the "download" widget

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Depends on 1 bug)

Trunk
Firefox 64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
The "download" widget is used by the Downloads Panel and the Downloads View, and it currently uses different button elements for each possible action.

These elements are shown or hidden using CSS styles that use the legacy "state" attribute. This is more difficult to understand and maintain than a solution based on changing the associated action in JavaScript.

Converting this to a Custom Element or a plain class will be much easier if all the elements don't have to be created programmatically.
(Assignee)

Updated

a year ago
Depends on: 1452637
(Assignee)

Updated

a year ago
Priority: -- → P3
(Assignee)

Updated

8 months ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: P3 → P1
(Assignee)

Comment 2

8 months ago
I'll land this after the merge. I'll use the same approach for the status labels, and maybe the context menu as well.
(Assignee)

Comment 3

8 months ago
Comment on attachment 9004529 [details]
Bug 1452629 - Part 1 - Avoid redundant button elements in the "download" widget. r=mak

In "downloadsStrings.inc.xul", I've assigned the attribute names using identifiers that resemble Fluent conventions, so we can just keep the same l10nId in the code. Do the identifier names look fine to you?
Attachment #9004529 - Flags: feedback?(gandalf)
(Assignee)

Comment 5

8 months ago
Comment on attachment 9004554 [details]
Bug 1452629 - Part 2 - Avoid redundant button label elements in the "download" widget. r=mak

Here I've added new identifiers with "-description" at the end, which are used on a different element. For one of them, I've added a new identifier even if the string is the same, since they're used in a different context.
Attachment #9004554 - Flags: feedback?(gandalf)
(Assignee)

Comment 6

8 months ago
The "Open File" message is now correctly displayed when hovering the one-pixel-wide clickable area around the action button.

Depends on D4447
Comment on attachment 9004529 [details]
Bug 1452629 - Part 1 - Avoid redundant button elements in the "download" widget. r=mak

Zibi Braniecki [:gandalf][:zibi] has approved the revision.
Attachment #9004529 - Flags: review+
Comment on attachment 9004554 [details]
Bug 1452629 - Part 2 - Avoid redundant button label elements in the "download" widget. r=mak

Zibi Braniecki [:gandalf][:zibi] has approved the revision.
Attachment #9004554 - Flags: review+
(Assignee)

Comment 13

8 months ago
I've added a refactoring commit to simplify the state handling after the changes made here, so we have just one instead of three different blocks that check the download state.

At the moment, I've only posted part of the refactoring, so the review can more easily follow each step, but I'll do more work and update Part 6 afterwards.
Comment on attachment 9004529 [details]
Bug 1452629 - Part 1 - Avoid redundant button elements in the "download" widget. r=mak

Zibi Braniecki [:gandalf][:zibi] has been removed from the revision.
Attachment #9004529 - Flags: review+
Comment on attachment 9004554 [details]
Bug 1452629 - Part 2 - Avoid redundant button label elements in the "download" widget. r=mak

Zibi Braniecki [:gandalf][:zibi] has been removed from the revision.
Attachment #9004554 - Flags: review+
(Assignee)

Updated

8 months ago
Attachment #9004529 - Flags: feedback?(gandalf) → feedback+
(Assignee)

Updated

8 months ago
Attachment #9004554 - Flags: feedback?(gandalf) → feedback+
Comment on attachment 9004529 [details]
Bug 1452629 - Part 1 - Avoid redundant button elements in the "download" widget. r=mak

Marco Bonardo [::mak] has approved the revision.
Attachment #9004529 - Flags: review+
Comment on attachment 9004554 [details]
Bug 1452629 - Part 2 - Avoid redundant button label elements in the "download" widget. r=mak

Marco Bonardo [::mak] has approved the revision.
Attachment #9004554 - Flags: review+
Comment on attachment 9004597 [details]
Bug 1452629 - Part 3 - Remove the "downloadOpenFile" description element. r=mak

Marco Bonardo [::mak] has approved the revision.
Attachment #9004597 - Flags: review+
Comment on attachment 9004598 [details]
Bug 1452629 - Part 4 - Remove the "downloadShowMoreInfo" description element. r=mak

Marco Bonardo [::mak] has approved the revision.
Attachment #9004598 - Flags: review+
(In reply to :Paolo Amadini from comment #13)
> At the moment, I've only posted part of the refactoring, so the review can
> more easily follow each step, but I'll do more work and update Part 6
> afterwards.

So, what's the current status of part 6?
Comment on attachment 9004599 [details]
Bug 1452629 - Part 5 - Remove the "downloadDetailsFull" description element. r=mak

Marco Bonardo [::mak] has approved the revision.
Attachment #9004599 - Flags: review+
(Assignee)

Comment 22

8 months ago
(In reply to Marco Bonardo [::mak] from comment #20)
> So, what's the current status of part 6?

I've just completed the refactoring and I'm posting the code for review right now. It may use some more code comments, let me know.
(Assignee)

Comment 23

8 months ago
I'll probably move the refactoring to a different bug, since it also does minor changes to New Tab Page code. Anyways, these will all land after the merge.
(Assignee)

Comment 25

7 months ago
In the latest version I've removed the string cache, following Zibi's recommendation.

Comment 26

7 months ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58fc7f243f76
Part 1 - Avoid redundant button elements in the "download" widget. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/de4eb68568a0
Part 2 - Avoid redundant button label elements in the "download" widget. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/4aca3cccf16f
Part 3 - Remove the "downloadOpenFile" description element. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e3334d170d
Part 4 - Remove the "downloadShowMoreInfo" description element. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/1de95b7783f0
Part 5 - Remove the "downloadDetailsFull" description element. r=mak
(Assignee)

Updated

7 months ago
Blocks: 1493969
Attachment #9004872 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.