1297657 - Correct the dropmarker design in Downloads Panel for RTL and plain style

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

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

Attachment: Screen Shot 2016-08-24 at 17.46.35.png

Based on the discussion at bug 1269962 comment 94 (thanks Dão!), this bug should fix the above issues.

(In reply to :Paolo Amadini from comment #95)
> (In reply to Dão Gottwald [:dao] from comment #94)
> > 1. Does this work with right-to-left locales?
> 
> Probably not, I missed this in the review. We should use
> "margin-inline-start".
> 
Please see the attachment and the padding/margin of "Show All Downloads" should be fixed.

> > 2. Does this work for languages other than English where the string has a
> > different width?
> 
> I think it should work correctly and wrap if needed, we can test with longer
> strings.
> 
UX/Visual team might have some thoughts for the long string case, and I will show them the screenshot for them to make the decision.
> > 3. Why !important?
> > 
> > I thought we had already established that the plain class is not the right
> > way to reset styles since it uses !important. It also doesn't do anything
> > about colors.
> 
> This was an existing pattern, but I agree that as a follow-up we should
> replace all uses of "plain" in the panel with our own styling overrides (and
> this removes the need for !important above). Sean, can you file a follow-up
> bug to implement Dão's suggestion and also fix the "margin-inline-start"?

Comment 1

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

Attachment: Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

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

Comment 2

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

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

Hey Dão, Paolo,

Since the longer string solution is discussing with visual designer, it's given 10px padding-inline-start/end as a workaround.

Besides, the reason to use padding-inline-start/end as padding-left/right is to prevent that padding-inline-start overrides padding-left/right in [hasdownloads] case.

Could you give this patch a feedback?
Thank you.

Comment 3

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

Attachment: RTL_fixed_screenshot

Comment 4

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

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

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

Comment 5

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

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

It's a follow-up in comment 2 with .downloadsPlain replacement for
richlistbox#downloadsListBox
button#downloadsPanel-blockedSubview-openButton
button#downloadsPanel-blockedSubview-deleteButton

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

Not sure the downloadsPlain class is a net simplification. Seems like it could be a footgun, since e.g. removing -moz-appearance may or may not leave behind a CSS background, and then you may or may not have to adjust the text color etc.

Can you just reset the properties you want to reset like you already did in .downloadsPanelFooterButton before you reversed it in this patch?

Comment 7

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

(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 8784753 [details]
> Bug 1297657 - Refine the plain style of buttons in Downloads Panel and
> support RTL.
> 
> Not sure the downloadsPlain class is a net simplification. Seems like it
> could be a footgun, since e.g. removing -moz-appearance may or may not leave
> behind a CSS background, and then you may or may not have to adjust the text
> color etc.
I am not sure if I misunderstand your point. :P
If -moz-appearance in downloadsPlain is not given "none", the buttons will be in a system style which is not an approach for similar style cross platform. Even some style changes for text-color, background, etc could be a must.

> 
> Can you just reset the properties you want to reset like you already did in
> .downloadsPanelFooterButton before you reversed it in this patch?
After checking .downloadsPanelFooterButton, probably these two css rules could be moved to .downloadsPlain as well.
  background-color: transparent;
  color: inherit;

Comment 8

[Dão Gottwald [:dao]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > Comment on attachment 8784753 [details]
> > Bug 1297657 - Refine the plain style of buttons in Downloads Panel and
> > support RTL.
> > 
> > Not sure the downloadsPlain class is a net simplification. Seems like it
> > could be a footgun, since e.g. removing -moz-appearance may or may not leave
> > behind a CSS background, and then you may or may not have to adjust the text
> > color etc.
> I am not sure if I misunderstand your point. :P
> If -moz-appearance in downloadsPlain is not given "none", the buttons will
> be in a system style which is not an approach for similar style cross
> platform. Even some style changes for text-color, background, etc could be a
> must.

I wasn't saying you shouldn't set -moz-appearance: none. I was saying you shouldn't use the downloadsPlain class to do it.

> > Can you just reset the properties you want to reset like you already did in
> > .downloadsPanelFooterButton before you reversed it in this patch?
> After checking .downloadsPanelFooterButton, probably these two css rules
> could be moved to .downloadsPlain as well.
>   background-color: transparent;
>   color: inherit;

That's not necessarily enough. E.g. richlistitems in the richlistbox could have their own background and/or text color that assume that richlistbox is styled in a certain way. I haven't checked whether they actually do, my point is that you're opening the door to new problems when this class gets used more widely.

So again, can you just reset properties directly without that class?

Comment 9

[:Paolo Amadini]

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

Yeah, unfortunately we need overrides that are specific to the control type.

With CSS, when there are too many classes on an element it may be difficult to control the priority of the rules, as it depends on their order in the file, so in the long run it may be difficult to use a generic class for "plain" even if it saves lines of code.

You can just add the needed property overrides, including "-moz-appearance", to "#downloadsListBox" and ".downloadsPanelFooterButton".

Comment 10

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

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

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

Comment 11

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

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

Hey Paolo, Dao,

Thanks for your explanation. I am sorry that I misunderstand Dão’s thoughts.

This patch fixes RTL issue and also padding/margin of .downloadsPanelFooterButton > .button-box to get the consistent button size between Windows/OSX/Linux.

Could you help to review the patch again? Thank you.

Comment 12

[:Paolo Amadini]

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

https://reviewboard.mozilla.org/r/74074/#review72368

::: browser/themes/shared/downloads/downloads.inc.css:114
(Diff revision 3)
> +.downloadsPanelFooterButton > .button-box {
> +  padding: 0;
> +  margin: 0;
> +  border: none;
>  }

Please keep in the order, this should be near the existing .downloadsPanelFooterButton rule.

Have you checked that the focus ring still displays correctly when using keyboard focus on various platforms?

::: browser/themes/shared/downloads/downloads.inc.css:128
(Diff revision 3)
> +  padding: 1px;
> +  margin: 1px;

I haven't tested yet, but I guess this should be zero since we already define padding on the rest of the button.

These are the two issues I could see, but I'll let Dão review the rest since he may have more opinions on the details of the implementation.

Comment 13

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

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74074/diff/3-4/

Comment 14

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

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

Hey Dão, could you help to review the patch with Paolo's suggestion? Thank you.

Comment 15

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #12)
> > +.downloadsPanelFooterButton > .button-box {
> > +  padding: 0;
> > +  margin: 0;
> > +  border: none;
> >  }
> 
> Please keep in the order, this should be near the existing
> .downloadsPanelFooterButton rule.
> 
> Have you checked that the focus ring still displays correctly when using
> keyboard focus on various platforms?

Could you please answer this question?

Comment 16

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

(In reply to Dão Gottwald [:dao] from comment #15)
> (In reply to :Paolo Amadini from comment #12)
> > Have you checked that the focus ring still displays correctly when using
> > keyboard focus on various platforms?
> 
> Could you please answer this question?

After enabling full keyboard access in OSX (bug 1296128 comment 7), Windows/Linux/OSX can display focus ring like dotted borders on Linux and Windows and blue borders on OSX.

However, the movement behavior in OSX is different with Windows/Linux.
For focusing between "Show All Downloads" and Dropmarker, "DOWN" and "RIGHT" keys should be pressed in Windows/Linux. But in OSX, should use "DOWN" and "UP" keys.

Shall this patch fix the different behavior?

Comment 17

[:Paolo Amadini]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #16)
> Shall this patch fix the different behavior?

If the behavior isn't changed by this patch, it doesn't need to be fixed in this bug.

Comment 18

[:Paolo Amadini]

Sean, can you test if the centering is still correct with the preference to hide the dropmarker?

Comment 19

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

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74074/diff/4-5/

Comment 20

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

(In reply to :Paolo Amadini from comment #18)
> Sean, can you test if the centering is still correct with the preference to
> hide the dropmarker?

In the latest patch, the class downloadsHideDropmarker will be checked when applying "padding-inline-start"
+#downloadsPanel[hasdownloads] #downloadsFooterButtons:not(.downloadsHideDropmarker) #downloadsHistory {
+  padding-inline-start: 68px;
 }

Thanks for finding the issue!

Comment 21

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

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

Hey Paolo, Dão,

Since there are some changes from last review, could you help to review the patch again? Thanks.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

>+#downloadsPanel[hasdownloads] #downloadsFooterButtons:not(.downloadsHideDropmarker) #downloadsHistory {

Please use the child selector.

Looks okay otherwise.

Comment 23

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

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74074/diff/5-6/

Comment 24

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

Hey Dao, could you help to give r+ again? sorry that the flag is cleared accidentally. I should update r? in comment earlier next time. sorry to bother you.

Comment 25

[Dão Gottwald [:dao]]

Can you use the child selector between #downloadsPanel[hasdownloads] and #downloadsFooterButtons too?

Comment 26

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

Hey Dão,

There are too many elements in the selector between #downloadsPanel[hasdownloads] and #downloadsFooterButtons. Do you think this is necessary?

Here is the selector using child selector without pseudo elements
#downloadsPanel[hasdownloads] #downloadsPanel-multiView #downloadsPanel-mainView > #downloadsFooter > stack > #downloadsFooterButtons:not(.downloadsHideDropmarker) > #downloadsHistory {

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 8784753 [details]
Bug 1297657 - Refine the plain style of buttons in Downloads Panel and support RTL.

Alright

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/704a8e3c5db3
Refine the plain style of buttons in Downloads Panel and support RTL. r=dao

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/704a8e3c5db3

Comment 30

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using 51.0a1, build ID: 20160824030337, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 51.0b13, build ID: 20170109165508 and Fx 52.0a2, build ID: 20170111004018 and Fx53.0a1, build ID: 20170110075905, on Windows 10 x64, Mac OS X 10.10.5 and Ubuntu 14.04 LTS.

Cheers!