Bug 760607 did increase the size of the icons. Now they are the size of the mouse pointer for me. That's still too small. It's easy to misunderstand the state of the download, and the icon is disproportionately small as a click target compared to the file entry. It is also smaller than the file type icon, which is, IMO, less useful (and not a button). Imagine clicking the wrong link and wanting to stop the download: the icon will change from "Cancel" to "Open" at the end, and you won't notice. I am using larger-than-default fonts, and a GTK theme (Shiki Dust) that makes the background a little darker. Compare attachment 694437 [details] and my screenshot.

(Remember that static screenshots of a part of an animated UI can't show the distraction and quick discoverability issues.)

once bug 991120 provides the assets, the scope of this bug is to split each richlistitem so that all of the right part of it activates the default action (see the mock-up in bug 963745)

Seems I can't reproduce this bug on linux. After adjusting system dpi, the icon also became bigger which is an expected behavior. Is this bug still happen? I remembered that we've gained better dppx support these year.

Same size with 2016-08-07-03-02-01-mozilla-central-firefox-51.0a1.ru.linux-x86_64.

Per comment 2, the remaining work here is only to split each richlistitem so that all of the right part of it activates the default action. I've updated the subject accordingly.

This bug should also take care that the two sections have the correct background hover effects defined in the visual specification. We may still be able to do this with the current PNG icons, but maybe we'll need the SVG icons from bug 1289139 to be able to use the final colors.

WIP for macOS. Still need to work for other OSs.

Marked CHE-MVP per FE study result + conclusion of project meeting this week.

This is the screenshot for the patch for reference. Icons on buttons are left on bug 1289139 to be changed. Paolo may you take a look on this first version of patch, thanks!

Comment on attachment 8788415 [details] Bug 950058 - Split each download item so that all of the right part of it activates the action. https://reviewboard.mozilla.org/r/76924/#review75434 Looks good! I'll try it locally and see how we can better handle the colors and the new SVG buttons. Since we're at a pretty good stage with both bugs, we may want to land them together. ::: browser/components/downloads/content/downloadsOverlay.xul (Diff revision 1) > ondragstart="DownloadsView.onDownloadDragStart(event);"/> > <description id="emptyDownloads" > mousethrough="always"> > &downloadsPanelEmpty.label; > </description> > - <spacer flex="1"/> This is required for the blocked downloads subview to work correctly.

Comment on attachment 8788415 [details] Bug 950058 - Split each download item so that all of the right part of it activates the action. https://reviewboard.mozilla.org/r/76924/#review75924 I tried this together with bug 1289139, and the new style looks really good! There might be a bit more testing to do on other platforms, for example on Windows 7 I noticed an extra light border that should be removed. According to the visual specification we should remove the use of the special highlight color for the background of the finished state. The main area should not be highlighted on hover when it's not clickable. In this case, only the button on the side should get the new light background when the pointer is in the main area, similarly to what we do with the current styling. In the final design, we'll only have this case when the file has been removed from disk, but for now it's true in basically all download states except the finished state and the "flagged by application reputation" state. The "flagged" case is strange because both areas have the same effect when clicked, so they should just be a single area, but I think this case is rare enough that, if we want to address it at all, it can be done in a follow-up bug or a separate changeset. We need the "active" states for both areas, used when the buttons are pressed, exactly like the footer does. The keyboard focus case should be aligned with the current practices of Firefox theming. We discussed in bug 1296128 and we got input from Stephen that, at least for now, we still want to keep the platform conventions for them. For this bug, it means that we should just do the same visual treatment that we do for the "show all downloads" button in the footer. This is a case where Carol might want to update the Downloads Panel visual design document to reflect the current general design of Firefox. There are some smaller items that we can review in the next iteration. For example I'm not sure about the exact colors to use for the red background, one thing I noticed is that the red circle of the blocked badge is lost on hover. We should get more input from Carol on this case. We probably also need to make both labels white, to have enough contrast and to avoid using a platform-defined text color on a non-platform-defined background. Another detail is that when opening the subview for flagged downloads, I think we should align the backwards arrow at the center of the left space. There is a method to specify the alignment of the anchor in <panelmultiview>. ::: browser/components/downloads/content/download.xml:53 (Diff revision 1) > <span class="indent">>></span> <xul:description class="downloadDetails" > <span class="indent">>></span> crop="end" > <span class="indent">>></span> xbl:inherits="value=status,tooltiptext=statusTip"/> > <span class="indent">>></span> </xul:vbox> > + </xul:hbox> > + <xul:toolbarseparator class="downloadsDropmarkerSplitter"></xul:toolbarseparator> The downloadsDropmarkerSplitter class is specific to the dropmarker, we should just use a general <toolbarseparator> here, or rename the class if it makes sense to simplify the CSS. ::: browser/themes/linux/downloads/downloads.css:46 (Diff revision 1) > background-image: linear-gradient(hsla(0,0%,100%,.1), transparent); > color: HighlightText; > cursor: pointer; > } > > -@notKeyfocus@ @itemFinished@[exists][verdict="Malware"]:hover, > +@notKeyfocus@ @itemDirty@[verdict="Malware"]:hover, @item@[verdict="Malware"] should be enough, and mirrors the rule below.

A quick report: To remove the highlight color, I need to also reset the icon that use inverted-colors and there are quit a bit rules to change. The rules make me a little bit confused and need to take some time to figure them out. Also this may cause some conflict with bug 1289139. I'm not sure if there's a good way to avoid this and may need some discussion before going further. Since we're having holidays in Taipei the next days, the work will be continued on next week.

Some WIP for reference. Things addressed are: - highlight background-color removal for both completed and key-navigation. - native-look border in mac osx for key-navigation. - no darker-colored hover for non-clickable item. - color for active state. - badge position change, aligned to spec. - background color change for malware items. (This is confirmed with Carol) Things not working correctly for now are: - win7 test. (I need to setup the VM) - Align for flagged downloads. - Changing button's icon due to removing highlighted colors. (work in progress) Hope I addressed all working items. For the last one, changing button's icons, I'm thinking if it's possible to leave it in another bug (maybe together with removing highlight color); That way we may have less issue when merging with bug 1289139.

Sorry this is the correct one.

Hey Rex, After rebasing the patch (see bug 1289139 comment 29), the background color in @item@[showingsubview] case needs to be changed to the new color. These are the possible rules relative to this issue: OSX: https://github.com/weilonge/gecko/blob/rexboy/DownloadsPanel/Bug950058_v2/browser/themes/osx/downloads/downloads.css#L40 Windows: https://github.com/weilonge/gecko/blob/rexboy/DownloadsPanel/Bug950058_v2/browser/themes/windows/downloads/downloads.css#L94 Linux: https://github.com/weilonge/gecko/blob/rexboy/DownloadsPanel/Bug950058_v2/browser/themes/linux/downloads/downloads.css#L39

More visual input is needed before taking action. If we decide to remove the highlight on subview, we can't distinguish which item is clicked.

After some discussion with Carol we'd keep the highlight color on subview.

I tried on using anchor for aligning button but failed to get a good result. Seems showSubView() aligns the center of an anchor element to the leftmost edge of the popup window. So clearly setting the button as anchor is not right because we want to align the center of the button to the rest of the remaining "ditch" outside the SubView. If I set the separator as an anchor, the result looks like this. Just not what we want either. So if we don't want to change the way of aligning anchor at this time, maybe we can leave this relatively minor issue for now.

Paolo I built it on windows 7 but not quite sure about what the light border is, do you mean the border that appeared when manipulating by keyboard?

(In reply to KM Lee [:rexboy] from comment #20) > Paolo I built it on windows 7 but not quite sure about what the light border > is, do you mean the border that appeared when manipulating by keyboard? It doesn't appear on this screenshot, maybe it was fixed by the latest patch or other changes, or appears only with some theme options. I'll test again when the latest version is ready for review, and I'll let you know if it still appears on my system. (In reply to KM Lee [:rexboy] from comment #19) > So if we don't want to change the way of aligning anchor at this time, maybe > we can leave this relatively minor issue for now. Sounds good, let's file a low priority polish bug for this issue. (In reply to KM Lee [:rexboy] from comment #14) > For the last one, changing button's icons, I'm thinking if it's possible to > leave it in another bug (maybe together with removing highlight color); That > way we may have less issue when merging with bug 1289139. I think the conclusion is that this bug should be tested and land together with bug 1289139, exactly because if we change the background colors we also need to change the foreground colors of the icons, and we can only do that with the SVG versions. So bug 1289139 should reflect whatever we do here for the background color.

Some rules for highlighting icons are removed; but since the icon changes will be done by bug 1289139, I kept some detailed state-changing works unchanged and let bug 1289139 finish that based on discussion with Sean.

Sean just told me he can't apply the patch so I did a rebase push. The patch itself is not changed.

push again for an one line change I missed (for completed download item on Mac). Not sure if it's something I missed or something wrong during rebasing... Sorry for bothering. Please use the latest version if you haven't start yet.

Thanks! I've added this change locally to the combined patches.

Driveby-nit: please rename item-info-box to something less generic. (I'm assuming downloads.css is not a scoped stylesheet and just loaded in browser.xul.)

Comment on attachment 8788415 [details] Bug 950058 - Split each download item so that all of the right part of it activates the action. https://reviewboard.mozilla.org/r/76924/#review79414 Okay, we're approaching the final behavior except that the dimming on hover should not happen on areas that are not clickable. I address this in more detail below. The general observation on the patches here and in bug 1289139 is that the resulting CSS is still much more complex and less readable than it could be. You and Sean should probably go through the final result and see if there are places where rules can be simplified, clustered, or reordered. This is independent from the more specific comments below, and should be done afterwards. We'll surely need another review pass after this one. (In reply to KM Lee [:rexboy] from comment #23) > Some rules for highlighting icons are removed; but since the icon changes > will be done by bug 1289139, I kept some detailed state-changing works > unchanged and let bug 1289139 finish that based on discussion with Sean. Actually, I think it's clearer if all these rules are removed in one go in the other patch, so this patch here is smaller and easier to manage. The two bugs must be tested together anyways. ::: browser/components/downloads/content/download.xml:18 (Diff revision 4) > <content orient="horizontal" > align="center" > onclick="DownloadsView.onDownloadClick(event);"> > + <xul:hbox class="item-info-box"> Call this class "downloadMainArea". You should move the align="center" to this element, so that the separator will properly stretch to the height of the download item without need for a rule for it. ::: browser/components/downloads/content/download.xml:53 (Diff revision 4) > <span class="indent">>></span> <xul:description class="downloadDetails" > <span class="indent">>></span> crop="end" > <span class="indent">>></span> xbl:inherits="value=status,tooltiptext=statusTip"/> > <span class="indent">>></span> </xul:vbox> > + </xul:hbox> > + <xul:toolbarseparator></xul:toolbarseparator> Just <xul:toolbarseparator />. You can refer to this element with "@item@ > toolbarseparator" in the CSS. ::: browser/components/downloads/content/download.xml:54 (Diff revision 4) > <span class="indent">>></span> crop="end" > <span class="indent">>></span> xbl:inherits="value=status,tooltiptext=statusTip"/> > <span class="indent">>></span> </xul:vbox> > + </xul:hbox> > + <xul:toolbarseparator></xul:toolbarseparator> > <xul:stack> Add a class="downloadButtonArea" to this stack. You'll need this for controlling the hover effect with transparent colors, see below. ::: browser/themes/linux/downloads/downloads.css:47 (Diff revision 4) > cursor: pointer; > } > > -@notKeyfocus@ @itemFinished@[exists][verdict="Malware"]:hover, > +@notKeyfocus@ @item@[verdict="Malware"]:hover, > @item@[showingsubview][verdict="Malware"] { > - background-color: hsl(4, 82%, 47%); > + background-color: hsl(6, 89%, 35%); Neither the current nor the new background colors match the style guide. If you want to update the color to be darker, I think #c21f09 is the one you want here (see <https://firefoxux.github.io/StyleGuide/#/colours>). ::: browser/themes/shared/downloads/download-blocked.svg:2 (Diff revision 4) > -<!-- This Source Code Form is subject to the terms of the Mozilla Public > - - License, v. 2.0. If a copy of the MPL was not distributed with this > - - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > -<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16" viewBox="0 0 16 16"> > - <style> > - circle { > - fill: #D92215; > - } > - rect { > +<!-- Generator: Adobe Illustrator 19.2.0, SVG Export Plug-In . SVG Version: 6.00 Build 0) --> > +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px" > + viewBox="0 0 16 16" style="enable-background:new 0 0 16 16;" xml:space="preserve"> > +<g id="Layer_1_1_"> > + <path fill="#d92015" d="M14.8,12.5L9.3,1.9C9,1.3,8.5,1,8,1S7,1.3,6.7,1.9L1.2,12.5c-0.3,0.6-0.3,1.2,0,1.7S2,15,2.6,15h10.8 > + c0.6,0,1.1-0.3,1.4-0.8C15.1,13.7,15.1,13.1,14.8,12.5z"/> > + <path fill="#fff" d="M8,11c-0.8,0-1.5,0.7-1.5,1.5S7.2,14,8,14s1.5-0.7,1.5-1.5S8.8,11,8,11z M8,10L8,10c0.6,0,1-0.4,1-1l0.2-4.2 > + c0-0.7-0.5-1.2-1.2-1.2S6.8,4.1,6.8,4.8L7,9C7,9.6,7.4,10,8,10z"/> > +</g> The SVG should be optimized, Sean has done something like this already so he may be able to help here. However, changing the position and shape of the badge requires some refactoring of how the margins are calculated to simplify it, even if the end result is the same. We should move all this work to another bug and do it later. Please remove the new SVG and associated CSS changes from this bug, and move them to a separate bug. ::: browser/themes/shared/downloads/downloads.inc.css:57 (Diff revision 4) > +richlistitem[type="download"]:hover { > + background-color: #ebebeb; > +} You need to avoid the hover effect when the cursor is over an item that cannot be clicked, but you still need to highlight the button, if present, when the mouse is over the main area of the download item. I solved this problem with these rules: @item@:hover .downloadButtonArea { background-color: var(--arrowpanel-dimmed); } @item@[verdict]:hover, @itemFinished@[exists]:hover { background-color: var(--arrowpanel-dimmed); } @item@[verdict] .downloadButtonArea, @itemFinished@[exists]:hover .downloadButtonArea { background-color: transparent; } Note the use of the new CSS variables for the hover effect, see bug 1301758 comment 2 for how they should be used. This solution isn't complete, you also need to handle the separator properly, but it is a start. Be sure to test permanently blocked downloads as well, they shouldn't get any hover effect or separator, just like deleted downloads. I was able to do this by pasting the URL <http://getstatuscode.com/450> in the Downloads Panel. ::: browser/themes/shared/downloads/downloads.inc.css:61 (Diff revision 4) > +richlistitem[type="download"]:hover toolbarseparator { > + height: var(--downloads-item-height); > +} You don't need this rule anymore after the XUL changes. You can use the existing rules for the separator: @item@ > toolbarseparator, toolbarseparator.downloadsDropmarkerSplitter { margin: 7px 0; } @item@:hover > toolbarseparator, #downloadsFooter:hover toolbarseparator.downloadsDropmarkerSplitter, #downloadsFooter[showingdropdown] toolbarseparator { margin: 0; } Again, this isn't the full solution as more states should be handled, just an idea of how the same problem can be solved with less rules. I assume you actually want the same 7px margin, if you need a different value then you can just use a different rule for it. ::: browser/themes/windows/downloads/downloads.css:70 (Diff revision 4) > @media (-moz-windows-default-theme) and (-moz-os-version: windows-vista), > (-moz-windows-default-theme) and (-moz-os-version: windows-win7) { > richlistitem[type="download"] { > border: 1px solid transparent; > border-bottom: 1px solid hsl(213,40%,90%); > } > } This is the rule that is causing the extra border and misalignment I saw on Windows 7, just remove it. Maybe when you tested on Windows 7 you weren't on the default theme?

When the panel styling is right, keep in mind that the same styles also need to be updated for the Library window so that it works in the same way. As you've seen, there are currently still two duplicate stylesheets, so the work must be done twice. This is still useful because, when it's done, there will be much less rules and it will be simpler to unify the two stylesheets into a single one.

Comment on attachment 8793650 [details] Bug 950058 - Split each download item so that all of the right part of it activates the action. Sorry that I add this attachment accidentally.

Comment on attachment 8788415 [details] Bug 950058 - Split each download item so that all of the right part of it activates the action. https://reviewboard.mozilla.org/r/76924/#review79414 > Neither the current nor the new background colors match the style guide. If you want to update the color to be darker, I think #c21f09 is the one you want here (see <https://firefoxux.github.io/StyleGuide/#/colours>). This color is specified by Carol (sorry I forget to address this). I just followed that and the spec should be updated in a few days. Should I tell her to change it again to match the style guide? > You need to avoid the hover effect when the cursor is over an item that cannot be clicked, but you still need to highlight the button, if present, when the mouse is over the main area of the download item. > > I solved this problem with these rules: > > @item@:hover .downloadButtonArea { > background-color: var(--arrowpanel-dimmed); > } > > @item@[verdict]:hover, > @itemFinished@[exists]:hover { > background-color: var(--arrowpanel-dimmed); > } > > @item@[verdict] .downloadButtonArea, > @itemFinished@[exists]:hover .downloadButtonArea { > background-color: transparent; > } > > Note the use of the new CSS variables for the hover effect, see bug 1301758 comment 2 for how they should be used. > > This solution isn't complete, you also need to handle the separator properly, but it is a start. > > Be sure to test permanently blocked downloads as well, they shouldn't get any hover effect or separator, just like deleted downloads. I was able to do this by pasting the URL <http://getstatuscode.com/450> in the Downloads Panel. For this I discussed with Carol, and by now she thinks it would be better to still highlight the item itself (but lighter than the one hovered on button). Since this bug is mainly for splitting button and there's actually another bugs (bug 1269964, bug 1282662) for that, would you mind if we move the polish and discussion works there?

For other changes I'll try to address comments first. Thanks for the suggestions!

(In reply to KM Lee [:rexboy] from comment #34) > This color is specified by Carol (sorry I forget to address this). I just > followed that and the spec should be updated in a few days. Should I tell > her to change it again to match the style guide? I think we want to follow the style guide, unless there is a reason to use a different color. Carol may clarify if there is any special reasoning for how the color in the patch was chosen, and if we should keep it or align to the style guide. > > You need to avoid the hover effect when the cursor is over an item that cannot be clicked, but you still need to highlight the button, if present, when the mouse is over the main area of the download item. > > Be sure to test permanently blocked downloads as well, they shouldn't get any hover effect or separator, just like deleted downloads. > > For this I discussed with Carol, and by now she thinks it would be better to > still highlight the item itself (but lighter than the one hovered on > button). One rule of all the interaction we have in the current Downloads Panel and other parts of Firefox is that if an item changes its appearance when the mouse cursor is over it, the item can be clicked. Doing otherwise gives the impression that Firefox is broken, because nothing happens on click. I think we should proceed without breaking the above rule, and if we really want to break that rule we should have a discussion, that is probably best handled in a separate bug. While keeping the rule above, in the current Downloads Panel we also have this special highlight mode for the action button that happens when the cursor is in the vicinity, but not exactly over the button. The mouse must then be moved exactly over the button for the click to have an effect, at which point we also change the way we highlight the button itself. The reason for this is that we want to provide a hint that the icon on the side is actually a button. With the new visual design, this is much more obvious and we may not need this hint anymore. To summarize, consider the following states: 1. No part of the item can be clicked, no separation is present 2. The entire item can be clicked, no separation is present 3. Both the main area and the button can be clicked 4. Only the button can be clicked The treatments for the first two cases are easy: 1. Never highlight on hover 2. Always use full highlight on hover The third case is already handled correctly by this version: 3. Use full highlight for the hovered part, and use light highlight for the other part How we handle the fourth case can be chosen between: 4(a). Use full highlight when hovering the button, and use light highlight on the button when hovering the main area 4(b). Use full highlight when hovering the button, and do nothing when hovering the main area In both cases, the main area is not clickable and so it is never highlighted. I originally suggested 4(a) but we may also do 4(b). Both options preserve the rule that the item that changes its appearance is the one that can be clicked. > Since this bug is mainly for splitting button and there's actually > another bugs (bug 1269964, bug 1282662) for that, would you mind if we move > the polish and discussion works there? I don't see how bug 1269964 and bug 1282662 are related to the styling changes, as they mention string and functional changes. As a side note, they also look more like meta-bugs to me. When it's time to implement them, then more specific bugs describing what is changing exactly should probably be filed.

(In reply to :Paolo Amadini from comment #36) > (In reply to KM Lee [:rexboy] from comment #34) > > > Since this bug is mainly for splitting button and there's actually > > another bugs (bug 1269964, bug 1282662) for that, would you mind if we move > > the polish and discussion works there? > > I don't see how bug 1269964 and bug 1282662 are related to the styling > changes, as they mention string and functional changes. As a side note, they > also look more like meta-bugs to me. When it's time to implement them, then > more specific bugs describing what is changing exactly should probably be > filed. Hi Paolo, bug 1269964 was fired for implementing the split main menu just like this bug (it's really not a meta bug in my original intention. :P ). I suppose this bug is just for splitting button and main area. If any other improvements can be discussed and implemented in bug 1269964, this patch would be smaller and landed first. I totally agree the 4 cases you mentioned are important and should have an agreement with Visual designer. I tried to break down all the works in main menu here: * Bug 950058 - Split the main area and action button of an item in main menu. It should provide a workable main menu. * Bug 1289139 - Use SVG for action buttons. * Bug 1269964 - Refine the foreground and background color in each state's hover behavior. Consider High Contrast issue as well. * Bug 1282662 - Refine the string/information in each state's hover behavior. * Bug 1282664 - Implement the new arrangement based on the new main menu item. IMHO, the patches in this bug and bug 1289139 are still worth to be landed because they provide a workable main menu with the new design. However, the current patches still are imperfect enough, and that's why Bug 1269964 and Bug 1282662 present to fix the color issues. If this bug and bug 1289139 are landed first based on the above scope, it probably give us more confidence to implement other bugs parallelly like bug 1269964 and bug 1282664. May I know your thoughts for the above the scope of each bug? Thank you!

Comment on attachment 8788415 [details] Bug 950058 - Split each download item so that all of the right part of it activates the action. https://reviewboard.mozilla.org/r/76924/#review79414 > This color is specified by Carol (sorry I forget to address this). I just followed that and the spec should be updated in a few days. Should I tell her to change it again to match the style guide? Just confirmed again with Carol. Actually Carol told me to use #aa1b08, which do in the Style Guide and the value here is converted from #aa1b08 to HSL by me since this seems to be a convention in that file.

(In reply to KM Lee [:rexboy] from comment #38) > which do in the Style Guide and the value here is converted from #aa1b08 to > HSL by me since this seems to be a convention in that file. For that value, my converter gives hsl(7, 91%, 35%), while you used hsl(6, 89%, 35%) which maps back to a different color. It's probably just easier to use the hex value so it's exact and it can be compared to the Style Guide.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #37) > May I know your thoughts for the above the scope of each bug? I think the next step for this bug and the SVG icons one is to get them updated to address the review comments, so that we can reason on code that is more easily readable. As far as the behavior goes, I'm not sure what you are suggesting. You mention a few other bugs in comment 37, but my understanding is that they encompass other functional changes (like displaying a message on hover) that have to be done in addition to the work in this bug. I never suggested to fold the functional work into this bug, so it's unclear to me why you mention them at all. The descriptions in comment 37, the actual subjects of the bugs, and their contents all say different things. We should discuss future work in another place, and here we should focus on styling only. With regard to styling, we should handle all the cases we currently have in the panel, and do that in a self-consistent way that makes sense when the patch lands. I agree we can make more changes on top of that later. Comment 36 shows how to handle those cases to hit the quality bar we need for landing these patches in mozilla-central, regardless of future work.

Comment on attachment 8788415 [details] Bug 950058 - Split each download item so that all of the right part of it activates the action. sry I'm going to test it with bug 1289139 first.

I hope all the necessary changes are addressed. This is tested together with WIP of bug 1289139. There will be a few lines of changes on there too. Let me note some noticeable changes not mentioned: - Some regressions are found on all downloads panel, so a few changes on xul and css are added to fix that. - Keyfocus on a malware item before this patch has slightly different appearances on different OSs. I tried to make them looks the same and move rules to shared css. I hope this is acceptable.

(In reply to KM Lee [:rexboy] from comment #44) > This is tested together with WIP of bug 1289139. There will be a few lines > of changes on there too. Thanks, I'll wait for the other patch to be posted so I can test the final behavior.

(In reply to :Paolo Amadini from comment #36) > (In reply to KM Lee [:rexboy] from comment #34) > > This color is specified by Carol (sorry I forget to address this). I just > > followed that and the spec should be updated in a few days. Should I tell > > her to change it again to match the style guide? > > I think we want to follow the style guide, unless there is a reason to use a > different color. Carol may clarify if there is any special reasoning for how > the color in the patch was chosen, and if we should keep it or align to the > style guide. The malware red color should follow the style guide. I've revise the color on the visual spec. Thanks for pointing that out! > > > You need to avoid the hover effect when the cursor is over an item that cannot be clicked, but you still need to highlight the button, if present, when the mouse is over the main area of the download item. > > > Be sure to test permanently blocked downloads as well, they shouldn't get any hover effect or separator, just like deleted downloads. > > > > For this I discussed with Carol, and by now she thinks it would be better to > > still highlight the item itself (but lighter than the one hovered on > > button). > > One rule of all the interaction we have in the current Downloads Panel and > other parts of Firefox is that if an item changes its appearance when the > mouse cursor is over it, the item can be clicked. Doing otherwise gives the > impression that Firefox is broken, because nothing happens on click. > > I think we should proceed without breaking the above rule, and if we really > want to break that rule we should have a discussion, that is probably best > handled in a separate bug. > > While keeping the rule above, in the current Downloads Panel we also have > this special highlight mode for the action button that happens when the > cursor is in the vicinity, but not exactly over the button. The mouse must > then be moved exactly over the button for the click to have an effect, at > which point we also change the way we highlight the button itself. The > reason for this is that we want to provide a hint that the icon on the side > is actually a button. With the new visual design, this is much more obvious > and we may not need this hint anymore. > > To summarize, consider the following states: > > 1. No part of the item can be clicked, no separation is present > 2. The entire item can be clicked, no separation is present > 3. Both the main area and the button can be clicked > 4. Only the button can be clicked > > The treatments for the first two cases are easy: > 1. Never highlight on hover > 2. Always use full highlight on hover > > The third case is already handled correctly by this version: > 3. Use full highlight for the hovered part, and use light highlight for the > other part > > How we handle the fourth case can be chosen between: > 4(a). Use full highlight when hovering the button, and use light highlight > on the button when hovering the main area > 4(b). Use full highlight when hovering the button, and do nothing when > hovering the main area > > In both cases, the main area is not clickable and so it is never > highlighted. I originally suggested 4(a) but we may also do 4(b). Both > options preserve the rule that the item that changes its appearance is the > one that can be clicked. The visual was based on Stephen's design that he created previously. http://people.mozilla.org/~shorlander/downloads-panel-updated/panel-download.html We could see the list item as one component. Using the divider is visually for the user easier to tell that left and right are different actions. The left is the item description and the right is the action. On the download panel, the button on the right always can be clicked for further action. Even the left column sometimes could not be clicked but the descriptions will change for most of the cases (to provide the user more info). So I think it make sense to keep the highlight without breaking the list items into many blocks of small items and become too complicated. We also have "active states", I believe the users could differentiate what items are clickable. Therefore, our design will go with 4(a). Use full highlight when hovering the button, and use light highlight on the button when hovering the main area Right now we don't have list item rules on the style guide. We might need to update the style guide later on. Let me know if you have any other concern. Thanks :) > > Since this bug is mainly for splitting button and there's actually > > another bugs (bug 1269964, bug 1282662) for that, would you mind if we move > > the polish and discussion works there? > > I don't see how bug 1269964 and bug 1282662 are related to the styling > changes, as they mention string and functional changes. As a side note, they > also look more like meta-bugs to me. When it's time to implement them, then > more specific bugs describing what is changing exactly should probably be > filed.

Comment on attachment 8788415 [details] Bug 950058 - Split each download item so that all of the right part of it activates the action. https://reviewboard.mozilla.org/r/76924/#review82228 Looks quite good, thanks! We need to add a rule to "browser/components/downloads/content/downloads.css" to hide the separator in the permanently blocked case, where all action buttons are hidden. ::: browser/themes/shared/downloads/allDownloadsViewOverlay.inc.css:68 (Diff revision 7) > > @item@[verdict="Uncommon"] .downloadBlockedBadge { > background-image: url("chrome://browser/skin/info.svg"); > } > > +@item@ toolbarseparator { Same comment for child selectors as in the other bug. ::: browser/themes/shared/downloads/downloads.inc.css:177 (Diff revision 7) > } > > richlistitem[type="download"] { > margin: 0; > - border-top: 1px solid var(--downloads-item-border-top-color); > border-bottom: 1px solid var(--downloads-item-border-bottom-color); In addition to removing the now unused top color variable, I guess that we want to remove the --downloads-item-border-bottom-color variable as well and just use a value that works for all platforms? ::: browser/themes/shared/downloads/downloads.inc.css:187 (Diff revision 7) > -richlistitem[type="download"]:first-child { > - border-top: 1px solid transparent; > +richlistitem[type="download"]:last-child { > + border-bottom: none; > } > > -richlistitem[type="download"]:last-child { > - border-bottom: 1px solid transparent; > +richlistitem[type="download"] > .downloadMainArea { > + height: var(--downloads-item-height); This shouldn't be needed because the main area should already stretch to the available height. ::: browser/themes/shared/downloads/downloads.inc.css:273 (Diff revision 7) > -.downloadButton > .button-box { > - padding: 0; > +@itemFinished@[exists]:hover .downloadMainArea, > +@item@:hover .downloadButton, Since the toolbarseparator is semi-transparent on common operating system themes, and here we're applying the highlight separately to the .downloadMainArea and the .downloadButtonArea with no outline, the result I see here on hover is that the separator inside the items is much lighter than the separator in the footer. Basically the one pixel area where the separator is does not get the dimmed background. Admittedly, this is a smaller issue that we may just fix in a follow-up. The CSS rules to do this precisely would be more complicated, like in the example I've given before. Feel free to file a follow-up bug if you don't think it's worth fixing this issue here. ::: browser/themes/shared/downloads/downloads.inc.css:295 (Diff revision 7) > +@keyfocus@ @itemFocused@[verdict="Malware"], > +@notKeyfocus@ @item@[verdict="Malware"]:hover, > +@notKeyfocus@ @item@[verdict="Malware"]:hover:active, > +@item@[showingsubview][verdict="Malware"] { > + background-color: #aa1b08; > + color: white; > +} (In reply to KM Lee [:rexboy] from comment #44) > - Keyfocus on a malware item before this patch has slightly different > appearances on different OSs. I tried to make them looks the same and move > rules to shared css. I hope this is acceptable. Unifying across platforms sounds good to me, thanks for doing that! I wonder, on keyboard navigation, maybe we should just show the normal focus ring like all other items, instead of inverting the item like we do on mouse hover? On OS X, for example, previously we inverted all items on keyboard navigation using the highlight color, so it made sense to invert malware as well. However, now we don't invert the items anymore, so maybe we shouldn't invert malware too. This behavior can be the same on all platforms, the focus ring style will change but it's already defined for each platform. If we want we can still invert on mouse hover independently, in other words remove the @notKeyfocus@ limitation.

(In reply to Carol Huang [:Carol] from comment #48) > Therefore, our design will go with 4(a). Use full highlight when hovering > the button, and use light highlight on the button when hovering the main area Thanks Carol for the detailed description! I've tested the latest patch by Rex that implements this and the result is really good! Thanks Rex!

Comment on attachment 8788415 [details] Bug 950058 - Split each download item so that all of the right part of it activates the action. https://reviewboard.mozilla.org/r/76924/#review82228 > Same comment for child selectors as in the other bug. ok, I'd check the similar cases across the patch. > In addition to removing the now unused top color variable, I guess that we want to remove the --downloads-item-border-bottom-color variable as well and just use a value that works for all platforms? Hmmm, right. I will just try to use the color specified in visual directly and remove these two variables then. > (In reply to KM Lee [:rexboy] from comment #44) > > - Keyfocus on a malware item before this patch has slightly different > > appearances on different OSs. I tried to make them looks the same and move > > rules to shared css. I hope this is acceptable. > > Unifying across platforms sounds good to me, thanks for doing that! > > I wonder, on keyboard navigation, maybe we should just show the normal focus ring like all other items, instead of inverting the item like we do on mouse hover? > > On OS X, for example, previously we inverted all items on keyboard navigation using the highlight color, so it made sense to invert malware as well. However, now we don't invert the items anymore, so maybe we shouldn't invert malware too. > > This behavior can be the same on all platforms, the focus ring style will change but it's already defined for each platform. > > If we want we can still invert on mouse hover independently, in other words remove the @notKeyfocus@ limitation. Sounds ok to me, but I'll sync with visual before making the change.

(In reply to KM Lee [:rexboy] from comment #51) > > In addition to removing the now unused top color variable, I guess that we want to remove the --downloads-item-border-bottom-color variable as well and just use a value that works for all platforms? > > Hmmm, right. I will just try to use the color specified in visual directly > and remove these two variables then. For this to work on all themes, we should either use one of the current transparent black colors, or --panel-separator-color if we're not interested in the distinction with the panel footer. > Sounds ok to me, but I'll sync with visual before making the change. Thanks!

I'm waiting on the new version here to review bug 1289139 as well.

I tried a little on using outline for solving the separator, but it leaves some issues that can't be solved perfectly. So I'll open a follow-up bug later. Thanks!

(In reply to KM Lee [:rexboy] from comment #55) > I tried a little on using outline for solving the separator, but it leaves > some issues that can't be solved perfectly. So I'll open a follow-up bug > later. Thanks! Sounds good! I've now finished the review and in bug 1289139 I've published my recommended changes for both bugs as a patch: https://reviewboard.mozilla.org/r/85288/diff/#index_header Same here, feel free to take and incorporate the parts that are relevant to this bug. For now I've only tested on Mac, please let me know if there is something that I missed for other platforms. I've adjusted the background colors in a few cases and simplified some rules too. I've seen that the highlight case when the subview is shown was very similar across platforms, so I've just unified it and I've removed the subtle gradient on Linux.

Hey Rex, I've split Paolo's review into two parts of Bug 950058 and Bug 1289139 [1]. Could you help to verify the changes and the result in these platforms? Thank you. [1] https://github.com/weilonge/gecko/commits/rexboy/DownloadsPanel/Bug950058_v5

Verified on my win7, mac, and linux. There's a small change to prevent separator from showing when switching to subview, but seems the solution has already included in Sean's patch so I just included the same solution in this patch. I'm not quite sure if I've tested -moz-windows-default-theme sucessfully; it looks good on my win7 anyway before/after applying the patch on comment #56 with default/classic/custom theme.

Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c20fcfa07f7 Split each download item so that all of the right part of it activates the action. r=paolo

Thanks Rex and Sean for this great work! I've landed the two bugs together on the mozilla-inbound integration branch. I did one last fix for the visibility of the separator when the file was removed from disk, for the rest I expect all the cases to be handled well, including high contrast mode support. If we find out later that something doesn't work as expected or the CSS needs refinements, it can be handled in follow-ups. While these patches doesn't change many things about the item structure, just for completeness I've run the browser-chrome regression tests for the folder locally, and they were successful.

Rex, can you file the follow-up bug from comment 35? Looks like on some monitors the separation is not really that visible.

OK, I've opened the bug for the color of separator. Thanks for the reminding!

Thanks!

I can confirm this issue is fixed on nightly. I verified using Fx 52.0a1 (20161030030204), on Windows 10 x64, Ubuntu 14.04 LTS and Mac 10.11. Cheers!