1287384 - Incorrect alignment of Summary section in Downloads panel

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[Jonathan Kingston [:jkt] he/him]

Attachment: 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.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]: visible ui regression on osx, we should make sure not to ship this on release unfixed.

Comment 2

[Marcia Knous [:marcia]]

Tracking 50+ for this visible Mac regression.

Comment 3

[Andrew Overholt [:overholt]]

How are things going with a potential fix here given we're now branching 50?

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Attachment: 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.

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Attachment: 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 6

[Sean Lee [:seanlee][:weilonge]]

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.

Comment 7

[:Paolo Amadini]

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?

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8777645 [details]
Bug 1287384 - Fix the progress bar aligment in Summary section of Downloads Panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69156/diff/1-2/

Comment 9

[Sean Lee [:seanlee][:weilonge]]

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.

Comment 10

[Sean Lee [:seanlee][:weilonge]]

[1] http://searchfox.org/mozilla-central/source/browser/themes/shared/downloads/downloads.inc.css#99

Comment 11

[:Paolo Amadini]

(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.

Comment 12

[:Paolo Amadini]

Attachment: Bug 1287384 - Fix icon and progress bar alignment in the Downloads Panel.

Review commit: https://reviewboard.mozilla.org/r/70906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70906/

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 14

[:Paolo Amadini]

(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?

Comment 15

[:Paolo Amadini]

(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.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Redirecting the question in https://bugzilla.mozilla.org/show_bug.cgi?id=1287384#c14 to flod.

Comment 17

[Francesco Lodolo [:flod]]

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.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/8cc027a0b80d
Fix icon and progress bar alignment in the Downloads Panel. r=jaws

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8cc027a0b80d

Comment 21

[Saheda Reza [:Antora]]

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

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Thanks!

Comment 23

[Andrew Overholt [:overholt]]

Can we uplift this fix to 50?

Comment 24

[:Paolo Amadini]

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

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9307efb6c537

Comment 27

[Subhrajyoti Sen]

Fixed on Firefox 50.0b3 using Linux Mint 18

Comment 28

[Vibhanshu Chaudhary]

Verified on Firefox 50.0b3 on Windows 10 Pro
[testday-20160930]

Comment 29

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Based on comment 27 and comment 28, I will set the corresponding flags.