Show an alert mark on badges if there are exceptional status to download files.

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Downloads Panel
P2
normal
VERIFIED FIXED
a year ago
5 months ago

People

(Reporter: rexboy, Assigned: rexboy)

Tracking

(Blocks: 1 bug, {feature})

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

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: [CHE-MVP])

MozReview Requests

()

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

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
UX Spec:
https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255193

We need to show an alert/warning mark if there are unexpected situation to download files. see the page of the UX spec above.

This bug should implement all kinds of marks mentioned.
(Assignee)

Updated

a year ago
Summary: Show an alert badge if there are exceptional status to download files. → Show an alert mark if there are exceptional status to download files.
(Assignee)

Updated

a year ago
Summary: Show an alert mark if there are exceptional status to download files. → Show an alert mark on badges if there are exceptional status to download files.
(Assignee)

Updated

a year ago
Assignee: nobody → rexboy
Whiteboard: [CHE-MVP]

Updated

a year ago
Priority: -- → P2
(Assignee)

Comment 1

a year ago
Turns out it's just some css changes?
Maybe I can have a patch tomorrow.

It's a little bit inconvenience to share the dotted-type badge with rounded-square type on the hamburger button if we need to position them differently. I'd ignore this little issue for now.
(Assignee)

Comment 2

a year ago
Created attachment 8773194 [details]
screenshot

This is the WIP screenshot under linux of warning.
(Assignee)

Comment 3

a year ago
Created attachment 8773195 [details] [diff] [review]
WIP
(Assignee)

Comment 4

a year ago
Hi :past
I'm trying to change the style of warning/danger badge that noted in the new UX spec, as you can see the screenshot for the new icon. But I found the rule for badge in Linux is somewhat different than OSX and Windows version; Almost all of the rule in Linux are with [cui-areatype="toolbar"]. I tried to remove some of them on my WIP patch, but not sure if I break something. Just want to confirm with you the original purpose for that rule, and is it possible to remove it and let it be the same as OSX and Windows? 
Thank you!
Flags: needinfo?(past)
I'm not sure why Linux is different, I just followed the existing pattern when I introduced the badges. I bet dao will know.
Flags: needinfo?(past) → needinfo?(dao+bmo)
(In reply to KM Lee [:rexboy] from comment #4)
> Hi :past
> I'm trying to change the style of warning/danger badge that noted in the new
> UX spec, as you can see the screenshot for the new icon. But I found the
> rule for badge in Linux is somewhat different than OSX and Windows version;
> Almost all of the rule in Linux are with [cui-areatype="toolbar"]. I tried
> to remove some of them on my WIP patch, but not sure if I break something.
> Just want to confirm with you the original purpose for that rule, and is it
> possible to remove it and let it be the same as OSX and Windows? 
> Thank you!

Please just keep the existing pattern (i.e. [cui-areatype="toolbar"] on Linux). I'm not sure about the historical reasons off-hand, but changing this seems way out of scope for this bug anyway.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
Comment on attachment 8778810 [details]
Bug 1282689 - Apply dot-style notification badge for downloading problematic files

Hello Paolo:

I made a first patch. Tested on Mac, Linux, and Windows;
Not sure if I'm doing things right, may you take a look?
Thanks!
Attachment #8778810 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 10

a year ago
try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8ed3a8d1da9
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
Sorry, I was asked for a very minor visual change. Will set reviewer back in a few minutes.
Comment hidden (mozreview-request)
(Assignee)

Comment 14

a year ago
Comment on attachment 8778810 [details]
Bug 1282689 - Apply dot-style notification badge for downloading problematic files

OK, sorry for the bothering. It's just some minor change on css pixel.
May you take a look on this patch, thanks!
Attachment #8778810 - Flags: review?(paolo.mozmail)

Comment 15

a year ago
(In reply to KM Lee [:rexboy] from comment #4)
> Almost all of the rule in Linux are with [cui-areatype="toolbar"]. I tried
> to remove some of them on my WIP patch, but not sure if I break something.

I've just checked this issue while reviewing the patch. The "cui-areatype" attribute lets you determine if the button is the small one in the toolbar or the large one in the panel controlled by the menu button.

Due to the current rules on Linux, the badge does not appear when the downloads button is moved to the panel controlled by the menu button. I don't think this is intentional, because the badge appears as expected on other platforms.

Since the scope of this bug is to get the badging right for the downloads button, we can fix the issue above in this bug. We don't necessarily need to fix this problem for all other badges, if it is much more work.

To control the size of the dot, 7px or 8px, you should use a rule based on the "cui-areatype" attribute instead of the one based on the "#nav-bar" ancestor that is currently in the patch.

Comment 16

a year ago
mozreview-review
Comment on attachment 8778810 [details]
Bug 1282689 - Apply dot-style notification badge for downloading problematic files

https://reviewboard.mozilla.org/r/69970/#review67676

I've tested the patch and it looks quite good! Here is a first round of comments for the code review.

::: browser/components/downloads/content/download.xml:87
(Diff revision 4)
>             extends="chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-badged">
>      <content>
>        <xul:stack class="toolbarbutton-badge-stack">
>          <children />
>          <xul:image class="toolbarbutton-icon" xbl:inherits="validate,src=image,label,consumeanchor"/>
> -        <xul:label class="toolbarbutton-badge" xbl:inherits="value=badge" top="0" end="0" crop="none"/>
> +        <xul:label class="toolbarbutton-badge" xbl:inherits="value=badge" top="5" end="4" crop="none"/>

For consistency with the menu button, it's better to keep these as "0", and override the margins in the CSS in the same way as the styling for the menu button.

::: browser/themes/linux/downloads/indicator.css:39
(Diff revision 4)
> +  min-width: 8px;
> +  border-radius: 4px;

You can use "min-width: 0;" and "border-radius: 50%" so you can declare these once, and don't need to change these when the size of the button changes.

::: browser/themes/linux/downloads/indicator.css:42
(Diff revision 4)
>    background-size: contain;
>    border: none;
>    box-shadow: none;
>    filter: drop-shadow(0 1px 0 hsla(206, 50%, 10%, .15));

We can probably remove these four properties, but I'll ask Panos about the filter in particular. So, the rule for the inactive window should not be necessary anymore.

::: browser/themes/shared/customizableui/panelUI.inc.css:135
(Diff revision 4)
> +#PanelUI-menu-button[badge-status="download-severe"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
> +  width: 7px;
> +  height: 7px;
> +  min-width: 7px;
> +  border-radius: 4px;
> +  transform: translate(-4px, 5px);

If you add "!important", you should be able to override the two margins declared in the base rule, instead of using a translation:

https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/toolkit/themes/windows/global/toolbarbutton.css#163-164

This should work correctly in RTL languages. You should add a comment that states that "!important" is necessary to override the rule in "toolbarbutton.css".
Attachment #8778810 - Flags: review?(paolo.mozmail)

Comment 17

a year ago
(In reply to :Paolo Amadini from comment #16)
> ::: browser/themes/linux/downloads/indicator.css:42
> (Diff revision 4)
> >    background-size: contain;
> >    border: none;
> >    box-shadow: none;
> >    filter: drop-shadow(0 1px 0 hsla(206, 50%, 10%, .15));
> 
> We can probably remove these four properties, but I'll ask Panos about the
> filter in particular. So, the rule for the inactive window should not be
> necessary anymore.

What was the original reason to override the box-shadow and use a filter instead? The original box-shadow improves contrast more, especially when the dot is yellow. If there is no reason to keep this, we should just remove these properties and use the default badge styling.
Flags: needinfo?(past)
I just reused the existing rules from bug 1180584 for consistency. It looks like there were some issues with getting the right shadow there (bug 1180584 comment 18). If it doesn't regress the FxA use case then we could change it back.
Flags: needinfo?(past)

Comment 19

a year ago
(In reply to Panos Astithas [:past] from comment #18)
> I just reused the existing rules from bug 1180584 for consistency. It looks
> like there were some issues with getting the right shadow there (bug 1180584
> comment 18). If it doesn't regress the FxA use case then we could change it
> back.

So, we should definitely use the default shadow styling for the Downloads badges because it gets the desired contrast for the new dot-style appearance.

For the Firefox Accounts badge, maybe the default styling would work as well and would get us more consistency, but we don't need to change it here necessarily. We can keep the existing custom shadow styling for the Firefox Accounts case.
(Assignee)

Comment 20

a year ago
Thanks for those suggestion, I do hesitated on positioning the badge for a while.
Let me reflect them in the patch and update. Also would try to change the shadow style.

Comment 21

a year ago
(In reply to :Paolo Amadini from comment #15)
> (In reply to KM Lee [:rexboy] from comment #4)
> > Almost all of the rule in Linux are with [cui-areatype="toolbar"]. I tried
> > to remove some of them on my WIP patch, but not sure if I break something.
> 
> I've just checked this issue while reviewing the patch. The "cui-areatype"
> attribute lets you determine if the button is the small one in the toolbar
> or the large one in the panel controlled by the menu button.
> 
> Due to the current rules on Linux, the badge does not appear when the
> downloads button is moved to the panel controlled by the menu button. I
> don't think this is intentional, because the badge appears as expected on
> other platforms.
> 
> Since the scope of this bug is to get the badging right for the downloads
> button, we can fix the issue above in this bug. We don't necessarily need to
> fix this problem for all other badges, if it is much more work.
> 
> To control the size of the dot, 7px or 8px, you should use a rule based on
> the "cui-areatype" attribute instead of the one based on the "#nav-bar"
> ancestor that is currently in the patch.

Driveby (I haven't tested the patch): when doing this, please ensure that this also displays correctly when using customize mode and when the button is in the overflow popup.
(Assignee)

Comment 22

a year ago
Created attachment 8781510 [details]
Shadow after removing the style

These are shadows after removing the customized styles.
(Assignee)

Comment 23

a year ago
Well, seems removing the shadow causes some inconsistency on badges between ordinary toolbar button and hamburger button (mainly for inactive state).
For example the one on Mac generates redundant border (see the screenshot on comment 22) for ordinary toolbar button.

Since the rules for filter, border, and box-shadow slightly different for each OSs, if we care about that, I may need to do some tracing to keep them consistent.
(Assignee)

Comment 24

a year ago
I can give it a try but if it takes too much time I'd tend to keep the shadow style as is.
Comment hidden (mozreview-request)
(Assignee)

Comment 26

a year ago
Comment on attachment 8778810 [details]
Bug 1282689 - Apply dot-style notification badge for downloading problematic files

Hey Paolo, I addressed the issues on the first version. May you take a look, thanks!
Attachment #8778810 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 27

a year ago
Some note:
There's a line under toolbarbutton.css that paints weird border on badge in OS X when window is inactive;
It came from bug 864894 for fixing some style issue.
I removed it after confirming it doesn't cause the same regression.
Comment hidden (mozreview-request)
(Assignee)

Comment 29

a year ago
Comment on attachment 8778810 [details]
Bug 1282689 - Apply dot-style notification badge for downloading problematic files

Corrected a few typo inside comments. Sorry for the confusing.
Attachment #8778810 - Flags: review?(paolo.mozmail)

Comment 30

a year ago
mozreview-review
Comment on attachment 8778810 [details]
Bug 1282689 - Apply dot-style notification badge for downloading problematic files

https://reviewboard.mozilla.org/r/69970/#review70656

Neat! We're almost there, just one change was missed.

::: browser/themes/osx/downloads/indicator.css:48
(Diff revision 6)
> +#nav-bar #downloads-button[attention="warning"] > .toolbarbutton-badge-stack > .toolbarbutton-badge,
> +#nav-bar #downloads-button[attention="severe"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
> +  height: 7px;
> +  width: 7px;
>  }

What I said in comment 15 is that '#nav-bar #downloads-button' should become '#downloads-button[cui-areatype="toolbar"]', both here and for Windows.

If you test as Gijs suggested in comment 21, you may note that using "#nav-bar" does not work if the download button is in the overflow popup, the one opened by the ">>" symbol that appears when you shrink the main window.

Probably, you can also take out the "[attention=]" part from the selector to make the rule simpler, because the width and height changes should not affect the badge when it's hidden, but I haven't tested this.

::: browser/themes/osx/downloads/indicator.css:54
(Diff revision 6)
> +#nav-bar #downloads-button[attention="severe"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
> +  height: 7px;
> +  width: 7px;
>  }
>  
> +

nit: This empty line addition can be reverted.
Attachment #8778810 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 31

a year ago
Comment on attachment 8778810 [details]
Bug 1282689 - Apply dot-style notification badge for downloading problematic files

Oh, I didn't realize overflow panel correctly so as a result I only tested the customize panel. Thanks for the detailed explaining.
The comments are addressed and tested. Thanks and please check it out.
Attachment #8778810 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Comment 33

a year ago
Comment on attachment 8778810 [details]
Bug 1282689 - Apply dot-style notification badge for downloading problematic files

Oops, it's this one.
Attachment #8778810 - Flags: review?(paolo.mozmail)

Comment 34

a year ago
mozreview-review
Comment on attachment 8778810 [details]
Bug 1282689 - Apply dot-style notification badge for downloading problematic files

https://reviewboard.mozilla.org/r/69970/#review71368

Looks good, thank you!
Attachment #8778810 - Flags: review?(paolo.mozmail) → review+

Updated

a year ago
Attachment #8773195 - Attachment is obsolete: true
(Assignee)

Comment 35

a year ago
Thank you Paolo!

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df044a4d73ec
(Assignee)

Comment 36

a year ago
Per discussion with Carol and Bryant, they think this patch is OK to land independently.
And the risk for this patch should be relatively low. I'll try to land it after try server runs ok.
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 37

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/031a2f4d9046
Apply dot-style notification badge for downloading problematic files r=Paolo
Keywords: checkin-needed

Comment 38

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/031a2f4d9046
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Assignee)

Updated

a year ago
Keywords: feature
KM Lee, is this something manual QA could help test?
Flags: qe-verify?
Flags: needinfo?(rexboy)
(Assignee)

Comment 40

a year ago
Yes. It's marked as feature based on our discussion earlier; but it's more like a visual polish.
You may refer to this UX page for what the badge specifically for:
https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255193
visual spec:
https://bug1269956.bmoattachments.org/attachment.cgi?id=8784314

And in case you need a mocked malware or unsure download file,
http://testsafebrowsing.appspot.com
Flags: needinfo?(rexboy)
(Assignee)

Comment 41

a year ago
(this bug corresponds to page 13-14 in the visual spec.)
Thank you for following up on this. We'll do the best we can to test this feature in Nightly, but since this was brought to our attention so late in the cycle, there's a risk that we won't be able to sign it off in time, before the merge to Aurora.

CC'ing Gerry as well, as a heads-up.
Flags: qe-verify? → qe-verify+
Actually, this is already being handled by the Engineering QA Team, so as far as I know, the sign off for pre-Aurora is on track.

Updated

10 months ago
Blocks: 1308189

Updated

10 months ago
Blocks: 1308232

Updated

10 months ago
Blocks: 1308237
I can confirm the alert marks on badges are properly displayed. I verified using Fx 50.0b1 (20161115182233) on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 LTS.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.