Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Downloads Panel
VERIFIED FIXED
11 months ago
6 months ago

People

(Reporter: seanlee, Assigned: seanlee)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified, firefox52 verified, firefox53 verified)

Details

(Whiteboard: [CHE-MVP])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

11 months ago
Created attachment 8784308 [details]
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"?
(Assignee)

Updated

11 months ago
Assignee: nobody → selee
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 2

11 months ago
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.
Attachment #8784753 - Flags: feedback?(paolo.mozmail)
Attachment #8784753 - Flags: feedback?(dao+bmo)
(Assignee)

Comment 3

11 months ago
Created attachment 8784757 [details]
RTL_fixed_screenshot
Comment hidden (mozreview-request)
(Assignee)

Comment 5

11 months ago
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
Attachment #8784753 - Flags: feedback?(paolo.mozmail)
Attachment #8784753 - Flags: feedback?(dao+bmo)

Comment 6

11 months ago
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?
Attachment #8784753 - Flags: feedback?(dao+bmo)
(Assignee)

Comment 7

11 months ago
(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

11 months ago
(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

11 months ago
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".
Attachment #8784753 - Flags: feedback?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

11 months ago
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.
Attachment #8784753 - Flags: review?(paolo.mozmail)
Attachment #8784753 - Flags: review?(dao+bmo)

Comment 12

11 months ago
mozreview-review
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.
Attachment #8784753 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

11 months ago
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.
Attachment #8784753 - Flags: review?(dao+bmo)

Comment 15

11 months ago
(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?
Flags: needinfo?(selee)
(Assignee)

Comment 16

11 months ago
(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?
Flags: needinfo?(selee)
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(dao+bmo)

Comment 17

11 months ago
(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.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(dao+bmo)

Comment 18

11 months ago
Sean, can you test if the centering is still correct with the preference to hide the dropmarker?
Comment hidden (mozreview-request)
(Assignee)

Comment 20

11 months ago
(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!
(Assignee)

Comment 21

11 months ago
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.
Attachment #8784753 - Flags: review?(paolo.mozmail)
Attachment #8784753 - Flags: review?(dao+bmo)

Updated

11 months ago
Attachment #8784753 - Flags: review?(paolo.mozmail)

Comment 22

11 months ago
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.
Attachment #8784753 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 24

11 months ago
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

11 months ago
Can you use the child selector between #downloadsPanel[hasdownloads] and #downloadsFooterButtons too?
(Assignee)

Comment 26

11 months ago
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 {
Flags: needinfo?(dao+bmo)

Comment 27

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

Alright
Flags: needinfo?(dao+bmo)
Attachment #8784753 - Flags: review?(dao+bmo) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 28

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/704a8e3c5db3
Refine the plain style of buttons in Downloads Panel and support RTL. r=dao
Keywords: checkin-needed

Comment 29

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/704a8e3c5db3
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Assignee)

Updated

11 months ago
Blocks: 1300947
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!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
status-firefox52: --- → verified
status-firefox53: --- → verified
You need to log in before you can comment on or make changes to this bug.