Closed Bug 1282689 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Downloads Panel, defect, P2)

Product:
Firefox
Component:
Downloads Panel
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- verified

People

(Reporter: rexboy, Assigned: rexboy)

References

Details

(Keywords: feature, Whiteboard: [CHE-MVP])

Attachments

(3 files, 1 obsolete file)

screenshot
41.24 KB, image/png
Details
Bug 1282689 - Apply dot-style notification badge for downloading problematic files
58 bytes, text/x-review-board-request
Paolo
: review+
Details
Shadow after removing the style
5.75 KB, image/png
Details 
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.
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.
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: nobody → rexboy
Whiteboard: [CHE-MVP]
Priority: -- → P2
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.
Attached image screenshot — 
This is the WIP screenshot under linux of warning.
Attached patch WIP (obsolete) — — Splinter Review 

    
    

    
  
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 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)

    
    

    
      
  

    
    

    
  
Sorry, I was asked for a very minor visual change. Will set reviewer back in a few minutes.

    
    

    
      
  

    
    

    
  
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)

    
    

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

    
    

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

    
    

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

    
    

    
  
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.

    
    

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

    
    

    
  


  
These are shadows after removing the customized styles.

    
    

    
  
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.

    
    

    
  
I can give it a try but if it takes too much time I'd tend to keep the shadow style as is.

    
    

    
      
  

    
    

    
  
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)

    
    

    
  
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 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 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)

    
    

    
  
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 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 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+

    
  

        Attachment #8773195 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/031a2f4d9046
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox51:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

    
    

    
  
KM Lee, is this something manual QA could help test?
Flags: qe-verify?
Flags: needinfo?(rexboy)

    
    

    
  
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)

    
    

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

    
  
Blocks: 1308189

    
  
Blocks: 1308232

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: