Closed
Bug 1452629
Opened 6 years ago
Closed 6 years ago
Avoid redundant elements in the "download" widget
Categories
(Firefox :: Downloads Panel, enhancement, P1)
Firefox
Downloads Panel
Tracking
()
RESOLVED
FIXED
Firefox 64
People
(Reporter: Paolo, Assigned: Paolo)
References
(Depends on 1 open bug)
Details
Attachments
(5 files, 1 obsolete file)
46 bytes,
text/x-phabricator-request
|
mak
:
review+
Paolo
:
feedback+
|
Details | Review |
46 bytes,
text/x-phabricator-request
|
mak
:
review+
Paolo
:
feedback+
|
Details | Review |
46 bytes,
text/x-phabricator-request
|
mak
:
review+
|
Details | Review |
46 bytes,
text/x-phabricator-request
|
mak
:
review+
|
Details | Review |
46 bytes,
text/x-phabricator-request
|
mak
:
review+
|
Details | Review |
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•6 years ago
|
Priority: -- → P3
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: P3 → P1
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years 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•6 years 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 4•6 years ago
|
||
Depends on D4441
Assignee | ||
Comment 5•6 years 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•6 years ago
|
||
The "Open File" message is now correctly displayed when hovering the one-pixel-wide clickable area around the action button. Depends on D4447
Assignee | ||
Comment 7•6 years ago
|
||
Depends on D4459
Assignee | ||
Comment 8•6 years ago
|
||
Depends on D4460
Comment 9•6 years ago
|
||
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 10•6 years ago
|
||
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 11•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca3233ac9c64efe6c39eb9e2ff3a0090c1d27b31
Assignee | ||
Comment 12•6 years ago
|
||
Depends on D4461
Assignee | ||
Comment 13•6 years 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 14•6 years ago
|
||
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 15•6 years ago
|
||
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•6 years ago
|
Attachment #9004529 -
Flags: feedback?(gandalf) → feedback+
Assignee | ||
Updated•6 years ago
|
Attachment #9004554 -
Flags: feedback?(gandalf) → feedback+
Comment 16•6 years ago
|
||
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 17•6 years ago
|
||
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 18•6 years ago
|
||
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 19•6 years ago
|
||
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+
Comment 20•6 years ago
|
||
(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 21•6 years ago
|
||
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•6 years 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•6 years 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 24•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ce4d95d7d746a7e6eeda0980b297dbeeef2d3b4
Assignee | ||
Comment 25•6 years ago
|
||
In the latest version I've removed the string cache, following Zibi's recommendation.
Comment 26•6 years 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
Comment 27•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/58fc7f243f76 https://hg.mozilla.org/mozilla-central/rev/de4eb68568a0 https://hg.mozilla.org/mozilla-central/rev/4aca3cccf16f https://hg.mozilla.org/mozilla-central/rev/48e3334d170d https://hg.mozilla.org/mozilla-central/rev/1de95b7783f0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Updated•6 years ago
|
status-firefox62:
--- → wontfix
status-firefox63:
--- → wontfix
Updated•6 years ago
|
Attachment #9004872 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•