Incorrect alignment of Summary section in Downloads panel

VERIFIED FIXED in Firefox 50

Status

()

P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: jkt, Assigned: Paolo)

Tracking

({good-first-bug, regression})

50 Branch
Firefox 51
good-first-bug, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 unaffected, firefox50+ verified, firefox51 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8771903 [details]
Screen Shot 2016-07-18 at 14.43.57.png

Sean Lee [:seanlee][:weilonge] 

The attachment is an screenshot for showing a bigger .downloadTypeIcon .
After reverting the codes mentioned by comment 27, the smaller icon is shown.
Is this a regression or new design? Thanks.

It can be reproduced in MacOSX but Ubuntu.
(Reporter)

Updated

3 years ago
Keywords: good-first-bug
[Tracking Requested - why for this release]: visible ui regression on osx, we should make sure not to ship this on release unfixed.
status-firefox49: --- → unaffected
status-firefox50: --- → affected
tracking-firefox50: --- → ?
Tracking 50+ for this visible Mac regression.
tracking-firefox50: ? → +
How are things going with a potential fix here given we're now branching 50?
Flags: needinfo?(selee)
Created attachment 8777644 [details]
progress_bar_alignment.png

The attachment shows the alignment of the progress bar in Summary section is still incorrect in m-c.

The correct one (above) is at hg.rev 401305ffbd994bd901ebf236dc5d3a0f69af51e4 .
The incorrect one (below) is at hg.rev 4a18b5cacb1b21a3e8b4b1dada6b2dd3dba51cb1 .

The patch will be provided soon.
Flags: needinfo?(selee)
Created attachment 8777645 [details]
Bug 1287384 - Fix the progress bar aligment in Summary section of Downloads Panel.

Review commit: https://reviewboard.mozilla.org/r/69156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69156/
Comment on attachment 8777645 [details]
Bug 1287384 - Fix the progress bar aligment in Summary section of Downloads Panel.

Hey Paolo,

Could you help to see the patch for fixing the alignment issue of Summary section? Thank you.
Attachment #8777645 - Flags: feedback?(paolo.mozmail)
Summary: Wrong icon size for download panel → Incorrect alignment of Summary section in Downloads panel
(Assignee)

Comment 7

3 years ago
https://reviewboard.mozilla.org/r/69156/#review66344

It looks like this is just fixing a symptom and not the cause.

Your note in bug 1265341 comment 28 sems closer to the right track. We use calc() to get the size of the icon, but we know we always want it to be 32x32 so this is what we should try to have in CSS.

Can you take a look and see if there are other things we can change so the negative margin in this patch is not necessary?
(Assignee)

Updated

3 years ago
Attachment #8777645 - Flags: feedback?(paolo.mozmail)
Comment hidden (mozreview-request)
Comment on attachment 8777645 [details]
Bug 1287384 - Fix the progress bar aligment in Summary section of Downloads Panel.

Sorry for the late update.

After removing the line [1], #downloadsSummaryChildBox can align with all xul:vbox.downloadContainer of richlistitem.download-state.
Compare #downloadsSummaryChildBox with xul:vbox.downloadContainer, only #downloadsSummaryChildBox has 12px margin-left (see in Box Model).
I suppose #downloadsSummaryChildBox and xul:vbox.downloadContainer should have very similar box model, so -moz-margin-start: var(--summary-padding-start); is removed.

However, --summary-padding-start seems relative to "rtl", and I am not sure if it's a correct solution.

Could you give the patch a feedback? Thank you.
Attachment #8777645 - Flags: feedback?(paolo.mozmail)
Attachment #8777645 - Flags: feedback?(jkt)
(Assignee)

Comment 11

3 years ago
(In reply to Sean Lee [:seanlee][:weilonge] from comment #9)
> I suppose #downloadsSummaryChildBox and xul:vbox.downloadContainer should
> have very similar box model, so -moz-margin-start:
> var(--summary-padding-start); is removed.

This is correct, the two elements should be styled in a similar way. One existing difference was in the way the width of the element was set using a localized entity only on the downloadContainer, while another cause of the misalignment was because of the way the margins of the two elements in the icon <stack> were defined.

Together with fixing the above two issues, most of the styles could be simplified to use margins instead of padding, so that calc() is not necessary anymore. I've created a patch that corrects all of this, and I'll submit it for review.

Note that I've reduced the horizontal margin between the icon and the downloadContainer by 4px, and I've also made the vertical margins symmetric so the icon is centered vertically again.

> However, --summary-padding-start seems relative to "rtl", and I am not sure
> if it's a correct solution.

You can use the "Force RTL" add-on to test the RTL layout easily. Some small visual bugs are still present when using the add-on compared to a real RTL build, but they can be ignored while testing a specific change.
Blocks: 1280825
(Assignee)

Updated

3 years ago
Attachment #8777645 - Attachment is obsolete: true
Attachment #8777645 - Flags: feedback?(paolo.mozmail)
Attachment #8777645 - Flags: feedback?(jkt)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Priority: -- → P1
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Comment on attachment 8780108 [details]
Bug 1287384 - Fix icon and progress bar alignment in the Downloads Panel.

https://reviewboard.mozilla.org/r/70906/#review69322

Tested out the patch, looks good to me.

::: browser/components/downloads/content/downloadsOverlay.xul:140
(Diff revision 1)
>                    onkeydown="DownloadsSummary.onKeyDown(event);"
>                    onclick="DownloadsSummary.onClick(event);">
>                <image class="downloadTypeIcon" />
> -              <vbox id="downloadsSummaryChildBox">
> +              <vbox pack="center"
> +                    class="downloadContainer"
> +                    style="width: &downloadDetails.width;">

I checked l10n-central and quite a few locales have customized the width of the downloads panel via this entity. Are you sure that the value will translate well to how it will be applied now?

https://dxr.mozilla.org/l10n-central/search?q=downloadDetails.width&redirect=false

It does seem that the name is no longer consistent with the element, since before it was on #downloadsSummaryDetails and now it is on .downloadContainer. If there is no difference, I wouldn't want to change the entity name for no significant reason because it will require all localizers to test this again.

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:35
(Diff revision 1)
> -  margin-inline-end: 8px;
> -  width: calc(var(--icon-size) + var(--inline-offset));
> +  margin: 8px 0;
> +  margin-inline-end: 12px;

I would prefer this to be written explicitly instead of overriding one of the properties immediately afterwards because someone may think this was a bug.

margin-top: 8px;
margin-inline-end: 12px;
margin-bottom: 8px;
margin-inline-start: 0;

::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:41
(Diff revision 1)
>  %ifdef XP_WIN
>  @media not all and (-moz-os-version: windows-xp) {
>    .downloadTypeIcon {
>      margin-inline-start: 8px;

Does this value need to be adjusted?

::: browser/themes/shared/downloads/downloads.inc.css:133
(Diff revision 1)
> -  --inline-offset: 8px;
> -  --block-offset: 4px;
> +  margin: 8px 0;
> +  margin-inline-end: 12px;

Same request here as in above about overriding properties.
Attachment #8780108 - Flags: review?(jaws) → review+
(Assignee)

Comment 14

2 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> I checked l10n-central and quite a few locales have customized the width of
> the downloads panel via this entity. Are you sure that the value will
> translate well to how it will be applied now?

I've checked the current Release and the progress bar in the footer is indeed misaligned on the right side, making it a bit longer. So this patch will remove space for one or two letters.

> If there is no difference, I wouldn't want to change the
> entity name for no significant reason because it will require all localizers
> to test this again.

There is a small difference indeed, but I'm not sure if we should request all localizers to test again. What do you think?
Flags: needinfo?(jaws)
(Assignee)

Comment 15

2 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> ::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:41
> (Diff revision 1)
> >  %ifdef XP_WIN
> >  @media not all and (-moz-os-version: windows-xp) {
> >    .downloadTypeIcon {
> >      margin-inline-start: 8px;
> 
> Does this value need to be adjusted?

I don't think it needs to be adjusted because the start margin hasn't changed, and seems to work fine where I have tested it. I'm not sure why that rule is there though, it might be related to differences in the Library window styling. If not, I hope that with the redesign in bug 950058 we'll be able to get rid of this difference.
Redirecting the question in https://bugzilla.mozilla.org/show_bug.cgi?id=1287384#c14 to flod.
Flags: needinfo?(jaws) → needinfo?(francesco.lodolo)
I've read the bug but I'm not completely sure what's the question for me.

Current values in Aurora: https://transvision.mozfr.org/string/?entity=browser/chrome/browser/downloads/downloads.dtd:downloadDetails.width&repo=aurora

What's the "small difference" compared to before? Is it safe to assume that the (tested) existing value will still be valid?

Also note that using a new ID would represent an obstacle for uplifting.
Flags: needinfo?(francesco.lodolo)
The small difference would be showing potentially 1 or 2 less characters. That is a good point about obstacles towards uplift.

I think we should keep the current sizes and not use a new ID.

Comment 19

2 years ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/fx-team/rev/8cc027a0b80d
Fix icon and progress bar alignment in the Downloads Panel. r=jaws

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8cc027a0b80d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I have successfully reproduced this bug on nightly according to(2016-07-18)

Fixing bug is verified on Latest Firefox Nightly-- Build ID: ( 20160826030226 ), User Agent 	Mozilla/5.0 (Windows NT 10.0; rv:51.0) Gecko/20100101 Firefox/51.0

Now this issue is solved on the new build.

Tested OS -- Windows10 32bit
QA Whiteboard: [testday-20160826]
Thanks!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Can we uplift this fix to 50?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 24

2 years ago
Comment on attachment 8780108 [details]
Bug 1287384 - Fix icon and progress bar alignment in the Downloads Panel.

Approval Request Comment
[Feature/regressing bug #]: Bug 1265341
[User impact if declined]: The icons near individual downloads, the icon in the footer and the progress bar shown when there are many downloads running are misaligned.
[Describe test coverage new/current, TreeHerder]: Landed on mozilla-central
[Risks and why]: We make the progress bar in the footer two characters smaller than Release, so some locales may theoretically display an ellipsis in some cases where they previously didn't, but this would probably already happen for regular downloads since we use a similar string. The progress in the footer is rarely displayed, so this is a small risk outweighted by the fact the patch fixes the download icon alignment in many more common cases.
[String/UUID change made/needed]: None
Flags: needinfo?(paolo.mozmail)
Attachment #8780108 - Flags: approval-mozilla-aurora?
Comment on attachment 8780108 [details]
Bug 1287384 - Fix icon and progress bar alignment in the Downloads Panel.

New regression in Fx50, fix was verified in Nightly, Aurora50+
Attachment #8780108 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 26

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/9307efb6c537
status-firefox50: affected → fixed
QA Whiteboard: [testday-20160826] → [testday-20160826] [good first verify]

Comment 27

2 years ago
Fixed on Firefox 50.0b3 using Linux Mint 18
Verified on Firefox 50.0b3 on Windows 10 Pro
[testday-20160930]
Based on comment 27 and comment 28, I will set the corresponding flags.
status-firefox50: fixed → verified
Version: unspecified → 50 Branch
You need to log in before you can comment on or make changes to this bug.