Comment on attachment 8882041 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/153120/#review158386 ::: browser/themes/linux/places/places.css:12 (Diff revision 3) > - margin: 0; > +#search-box { > + padding: 0; > } > > -.sidebar-placesTreechildren::-moz-tree-cell(leaf) , > -.sidebar-placesTreechildren::-moz-tree-image(leaf) { > +#search-box .textbox-search-icons { > + display: none; The search icon is moved to the other side in the mockups and the clear icon is still useful. We shouldn't remove these icons. Could just leave this as is for now. ::: browser/themes/linux/places/places.css:17 (Diff revision 3) > - cursor: pointer; > } > > -.sidebar-placesTreechildren::-moz-tree-cell-text(leaf, hover) { > - cursor: pointer; > - text-decoration: underline; > +.sidebar-placesTree { > + -moz-appearance: none; > + border: 0; Why did you remove the hover feedback? The tree feels pretty broken without it.

Seems like the UX spec is to remove the hover styling. I added back the magnifying glass/clear icons, but they'll need updating.

(In reply to Nihanth Subramanya [:nhnt11] from comment #6) > Seems like the UX spec is to remove the hover styling. I'm not ready to r+ this since it still looks broken to me. Stephen, what's the rationale behind that change?

Comment on attachment 8882041 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/153120/#review158762 Even more questionable changes here. I'd like to r+ a patch with just the padding, border and row height changes so we can get these landed. ::: browser/themes/linux/places/places.css:23 (Diff revision 5) > -.sidebar-placesTreechildren::-moz-tree-image(leaf) { > + min-height: 24px; > - cursor: pointer; > } > > -.sidebar-placesTreechildren::-moz-tree-cell-text(leaf, hover) { > - cursor: pointer; > +.sidebar-placesTreechildren::-moz-tree-row(selected) { > + background-color: rgba(0,0,0,.1); I understand this is in the mockup, but I think this unnecessarily throws away basic OS integration. I don't see what's wrong with using the native hihglight for selected rows. rgba(0,0,0,.1) also won't work with dark OS themes. Not your fault, but I (or somebody) will need to discuss this with Stephen. ::: browser/themes/linux/places/places.css:24 (Diff revision 5) > - cursor: pointer; > } > > -.sidebar-placesTreechildren::-moz-tree-cell-text(leaf, hover) { > - cursor: pointer; > - text-decoration: underline; > +.sidebar-placesTreechildren::-moz-tree-row(selected) { > + background-color: rgba(0,0,0,.1); > + border: none; This removes the focus ring, which is actually a usueful indicator, because items can be selected without being focused. ::: browser/themes/linux/places/places.css:28 (Diff revision 5) > - cursor: pointer; > - text-decoration: underline; > + background-color: rgba(0,0,0,.1); > + border: none; > } > > -.sidebar-placesTreechildren::-moz-tree-cell(separator) { > - cursor: default; > +.sidebar-placesTreechildren::-moz-tree-cell-text(selected) { > + color: black; This is not right at all, considering that OS themes may use different text colors.

(In reply to Dão Gottwald [::dao] from comment #7) > (In reply to Nihanth Subramanya [:nhnt11] from comment #6) > > Seems like the UX spec is to remove the hover styling. > > I'm not ready to r+ this since it still looks broken to me. Stephen, what's > the rationale behind that change? I forgot to add hover styling for Windows and Linux. macOS still doesn't have it (platform convention that we should respect here). Also focus styling was missing. I added them to the interactive mockup.

Comment on attachment 8887473 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/158326/#review163666 ::: browser/themes/linux/places/places.css:15 (Diff revision 2) > + > +#search-box { > + -moz-appearance: none; > + border: 1px solid hsla(240, 5%, 5%, 0.2); > + border-radius: 2px; > +} Any particular reason for dropping the native textbox appearance here? ::: browser/themes/linux/places/places.css:26 (Diff revision 2) > +#viewButton { > + -moz-appearance: none; > + border-radius: 4px; > + border: 1px solid transparent; > + padding: 2px 4px; > +} This breaks the button's focus ring. ::: browser/themes/linux/places/places.css:30 (Diff revision 2) > + padding: 2px 4px; > +} > + > +#viewButton:hover { > + background: hsla(240, 5%, 5%, 0.05); > + border-color: rgba(0, 0, 0, 0.2); Given bug 1374812, we probably shouldn't add this border. ::: browser/themes/linux/places/places.css:43 (Diff revision 2) > .sidebar-placesTree { > margin: 0; > + -moz-appearance: none; > + border: 0; > + background: transparent; > +} The color is still -moz-fieldtext here to match the -moz-field background that you removed. Please update the color accoringly. color: inherit should work here. ::: browser/themes/linux/places/places.css:46 (Diff revision 2) > + border: 0; > + background: transparent; > +} > + > +.sidebar-placesTreechildren::-moz-tree-row { > + height: 24px; min-height?

Comment on attachment 8887473 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/158326/#review163806 ::: browser/themes/linux/places/places.css:24 (Diff revisions 2 - 3) > } > > #viewButton { > -moz-appearance: none; > border-radius: 4px; > - border: 1px solid transparent; > + padding: 3px 5px; Removing the border results in a smaller visual size, so I increased the padding to compensate. ::: browser/themes/linux/places/places.css:28 (Diff revisions 2 - 3) > border-radius: 4px; > - border: 1px solid transparent; > - padding: 2px 4px; > + padding: 3px 5px; > +} > + > +#viewButton:focus { > + outline: 1px dotted -moz-DialogText; -moz-appearance: none; breaks the focus ring, added it back manually (based on http://searchfox.org/mozilla-central/source/browser/themes/linux/downloads/downloads.css#19)

Comment on attachment 8887473 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/158326/#review163666 > Any particular reason for dropping the native textbox appearance here? Just to match the spec (and be consistent within Firefox I guess?)

Comment on attachment 8887473 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/158326/#review164034 ::: browser/themes/linux/places/places.css:15 (Diff revision 4) > + > +#search-box { > + -moz-appearance: none; > + border: 1px solid hsla(240, 5%, 5%, 0.2); > + border-radius: 2px; > +} (In reply to Nihanth Subramanya [:nhnt11] from comment #16) > Comment on attachment 8887473 [details] > Bug 1377003 - [Photon] Update sidebar tree styling on Linux. > > https://reviewboard.mozilla.org/r/158326/#review163666 > > > Any particular reason for dropping the native textbox appearance here? > > Just to match the spec (and be consistent within Firefox I guess?) Please drop this change. We cannot easily use the native appearance in the location and search bars since they're highly customized. This doesn't mean that we should do the same for all textboxes in Firefox. I don't see the point here; your custom styling seems to mimic the native appearance on Ubuntu except that it's slightly off, and will be more off with different Gtk themes.

Comment on attachment 8887473 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/158326/#review166636 ::: browser/themes/linux/places/places.css:17 (Diff revision 5) > + -moz-appearance: none; > + border-radius: 4px; > + box-sizing: border-box; > + border: 1px solid transparent; > + padding: 2px 4px; > +} Please add color: inherit here. ::: browser/themes/linux/places/places.css:19 (Diff revision 5) > + box-sizing: border-box; > + border: 1px solid transparent; > + padding: 2px 4px; > +} > + > +#viewButton:focus:not(:-moz-any(:hover, :active, [open])) { Please use :-moz-focusring rather than :focus. And at this point :not(:-moz-any(:hover, :active, [open])) can probably be removed? ::: browser/themes/linux/places/places.css:20 (Diff revision 5) > + border: 1px solid transparent; > + padding: 2px 4px; > +} > + > +#viewButton:focus:not(:-moz-any(:hover, :active, [open])) { > + border: 1px dotted -moz-DialogText; Use outline rather than border? You should then remove border: 1px solid transparent; and presumably box-sizing: border-box; too. ::: browser/themes/linux/places/places.css:49 (Diff revision 5) > + fill: HighlightText; > } > > .sidebar-placesTreechildren::-moz-tree-image(container,selected) { > fill: HighlightText; > } This rule is now duplicated, please make sure you don't add it a second time here.

Comment on attachment 8887473 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/158326/#review168438 ::: browser/themes/linux/places/places.css:16 (Diff revision 5) > +#viewButton { > + -moz-appearance: none; > + border-radius: 4px; > + box-sizing: border-box; > + border: 1px solid transparent; > + padding: 2px 4px; I'm going to change this to `padding: 5px 4px 6px;` This matches the paddings in the search box.

Comment on attachment 8887473 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/158326/#review166636 > Use outline rather than border? You should then remove border: 1px solid transparent; and presumably box-sizing: border-box; too. It's got a border-radius, so outline is ugly.

(In reply to Nihanth Subramanya [:nhnt11] from comment #21) > Comment on attachment 8887473 [details] > Bug 1377003 - [Photon] Update sidebar tree styling on Linux. > > https://reviewboard.mozilla.org/r/158326/#review168438 > > ::: browser/themes/linux/places/places.css:16 > (Diff revision 5) > > +#viewButton { > > + -moz-appearance: none; > > + border-radius: 4px; > > + box-sizing: border-box; > > + border: 1px solid transparent; > > + padding: 2px 4px; > > I'm going to change this to > > `padding: 5px 4px 6px;` > > This matches the paddings in the search box. Not sure why these paddings should be the same. (In reply to Nihanth Subramanya [:nhnt11] from comment #23) > Comment on attachment 8887473 [details] > Bug 1377003 - [Photon] Update sidebar tree styling on Linux. > > https://reviewboard.mozilla.org/r/158326/#review166636 > > > Use outline rather than border? You should then remove border: 1px solid transparent; and presumably box-sizing: border-box; too. > > It's got a border-radius, so outline is ugly. Use -moz-outline-radius?

(In reply to Dão Gottwald [::dao] from comment #24) > (In reply to Nihanth Subramanya [:nhnt11] from comment #21) > > Comment on attachment 8887473 [details] > > Bug 1377003 - [Photon] Update sidebar tree styling on Linux. > > > > https://reviewboard.mozilla.org/r/158326/#review168438 > > > > ::: browser/themes/linux/places/places.css:16 > > (Diff revision 5) > > > +#viewButton { > > > + -moz-appearance: none; > > > + border-radius: 4px; > > > + box-sizing: border-box; > > > + border: 1px solid transparent; > > > + padding: 2px 4px; > > > > I'm going to change this to > > > > `padding: 5px 4px 6px;` > > > > This matches the paddings in the search box. > > Not sure why these paddings should be the same. I find it more aesthetically pleasing when the button is the same size as the search bar next to it. > (In reply to Nihanth Subramanya [:nhnt11] from comment #23) > > Comment on attachment 8887473 [details] > > Bug 1377003 - [Photon] Update sidebar tree styling on Linux. > > > > https://reviewboard.mozilla.org/r/158326/#review166636 > > > > > Use outline rather than border? You should then remove border: 1px solid transparent; and presumably box-sizing: border-box; too. > > > > It's got a border-radius, so outline is ugly. > > Use -moz-outline-radius? Nice, thanks.

Dao, you're the more seasoned Linux user between us; can you explicitly sign off or veto the view button size change to match the search bar?

Comment on attachment 8887473 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/158326/#review168920 ::: browser/themes/linux/places/places.css:15 (Diff revision 7) > + > +#viewButton { > + -moz-appearance: none; > + border-radius: 4px; > + -moz-outline-radius: 4px; > + padding: 6px 5px 7px; So you're trying to let the button and the textbox have the same height, right? Can you just remove align="center"? http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/browser/components/places/content/history-panel.xul#47 ::: browser/themes/linux/places/places.css:19 (Diff revision 7) > + -moz-outline-radius: 4px; > + padding: 6px 5px 7px; > + color: inherit; > +} > + > +#viewButton:-moz-focusring { if you add :not(:hover) here, or :not(:-moz-focusring) to the hover rule, you can get rid of the outline radius (it looks kind of ugly)

Comment on attachment 8887473 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/158326/#review168920 > So you're trying to let the button and the textbox have the same height, right? Can you just remove align="center"? http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/browser/components/places/content/history-panel.xul#47 I tried this, but it means adding margins to the view button on each platform. I'd rather just make the padding change on Linux.

(In reply to Nihanth Subramanya [:nhnt11] from comment #30) > Comment on attachment 8887473 [details] > Bug 1377003 - [Photon] Update sidebar tree styling on Linux. > > https://reviewboard.mozilla.org/r/158326/#review168920 > > > So you're trying to let the button and the textbox have the same height, right? Can you just remove align="center"? http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/browser/components/places/content/history-panel.xul#47 > > I tried this, but it means adding margins to the view button on each > platform. I'd rather just make the padding change on Linux. I think what we should do in addition to removing align="center": - consistently add the padding to #sidebar-search-container across platforms - remove the textbox's margin across platforms As I understand it, the view button shouldn't have margin on any platform.

(In reply to Dão Gottwald [::dao] from comment #31) > I think what we should do in addition to removing align="center": > - consistently add the padding to #sidebar-search-container across platforms > - remove the textbox's margin across platforms > > As I understand it, the view button shouldn't have margin on any platform. Well, except for a margin-inline-start to separate it from the textbox.

Comment on attachment 8896964 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/168268/#review173460 ::: browser/themes/linux/places/places.css:18 (Diff revisions 2 - 3) > } > > #viewButton { > -moz-appearance: none; > border-radius: 4px; > + margin: 1px 0; #search-box has a 1px gap above/below it even with 0 margin (and 0 padding on #sidebar-search-container). I suspect this comes from the OS specific text box styling, but I couldn't figure out what exactly is causing this. Added 1px top/bottom margin to the view button to match this for alignment.

Comment on attachment 8896964 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/168268/#review173904 ::: browser/themes/linux/places/places.css:18 (Diff revision 3) > +} > + > +#viewButton { > + -moz-appearance: none; > + border-radius: 4px; > + margin: 1px 0; margin-inline-start is missing here

Comment on attachment 8896964 [details] Bug 1377003 - [Photon] Update sidebar tree styling on Linux. https://reviewboard.mozilla.org/r/168268/#review174422

Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8943ddaccf14 [Photon] Update sidebar tree styling on Linux. r=dao

Hi Nihanth, can you please tell us what we need to verify here? Thanks

(In reply to ovidiu boca[:Ovidiu] from comment #43) > Hi Nihanth, can you please tell us what we need to verify here? Thanks Hi, the patch in this bug should change the history/bookmarks sidebar to more closely match the spec[1] on Linux. It does not completely match the spec, but updates the colors/margins/paddings especially within the bookmarks/history list. It also updates the view button styling in the history sidebar. Please feel free to needinfo? me about any discrepancies and we can clarify before this is marked Verified. [1] http://design.firefox.com/people/shorlander/photon/Mockups/linux.html

I verified this on Ubuntu 16.04 with Nightly 57.0a1(2017-08-22) and I saw that the buttons from bookmark toolbar/bookmark menu/other bookmarks don't look the same as in spec. This issue it wasn't updated I suppose, right? I can mark this verified or should I wait until all the sidebar respects the spec? Thanks

I was referring to the arrow buttons that are near the Bookmarks Toolbar, Bookmarks Menu Here is a print screen with the actual results: http://imgur.com/a/ILXor

Yup, the arrow button styling is a known issue and is blocked by bug 1381453.

(In reply to Nihanth Subramanya [:nhnt11] from comment #47) > Yup, the arrow button styling is a known issue and is blocked by bug 1381453. Should we mark bug 1381453 as a blocker for this one(bug1377003)?

I don't think so. That bug already blocks bug 1377011, which is the bug about updating icons in the sidebar tree for all platforms. This bug is about Linux-specific changes, so I think things are fine.

Thanks, I will mark this as verified based on comment 45 and comment 47.