1389167 - No obvious customization region for bookmarks toolbar in customize mode

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: regression

Reproducible: always

Steps To Reproduce:
1. Enter customize mode
2. Enable Bookmarks Toolbar
3. Optionally, remove "Bookmarks Toolbar Item"

Actual Results:
There is no bottom border or background color for the toolbar.
So not clear customizable region where additional toolbutton can drop.
Especially, after step3, I can not recognize the tool bar any longer.


Expected Results:
Provide border or background color for the toolbar at lest.

Comment 1

[Alice0775 White]

Attachment: screenshot

Comment 2

[:Gijs (out for now; he/him)]

Please triage this within the visual team. You removed the border. I don't know in what bug (I have tried to find it before now and failed). But presumably, when this was removed, there was some decision about this case. I am not aware of it, and, not knowing in which bug this happened, I don't know what the decision was or what to do with this bug.

Comment 3

[Dão Gottwald [:dao]]

I removed the border in bug 1349555, because other changes made that easy, it's part of <https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229252091> and I figured it would be welcome if I take this off photon-structure's plate. I don't intend to spend more time on this as it's not part of photon-visual, but I can add the border back if that's what you want.

Comment 4

[:Gijs (out for now; he/him)]

Aaron, why are we removing this border, and what are we supposed to be doing about the bookmarks toolbar? (please see the attached screenshot)

Comment 5

[Aaron Benson]

The border was removed to help make it as clear as possible that toolbar items could be dragged from the grid into the toolbar (a border might inhibit this connection). 

The bookmarks toolbar, however, is another issue and we'll address this after 57. Ideally this would be an actual toolbar with the bookmarks in it (and not the placeholder item that is currently shown).

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Aaron Benson from comment #5)
> The border was removed to help make it as clear as possible that toolbar
> items could be dragged from the grid into the toolbar (a border might
> inhibit this connection). 
> 
> The bookmarks toolbar, however, is another issue and we'll address this
> after 57. Ideally this would be an actual toolbar with the bookmarks in it
> (and not the placeholder item that is currently shown).

You think we should just ship this as-is? I don't think the state in the screenshot is shippable.

Comment 7

[Dão Gottwald [:dao]]

I suggest we (re?)add a dashed outline around the bookmarks toolbar while customizing. This should be good enough for users who've enabled the bookmarks toolbar without compromising the experience for the rest.

Comment 8

[Aaron Benson]

Here are some details for making the toolbar area more visible: 

Toolbar w/ contents: https://mozilla.invisionapp.com/share/FKD366GDM#/248932284_Customize_-_Toolbar_On
Toolbar empty: https://mozilla.invisionapp.com/share/FHD366WTU#/248932326_Customize_-_Toolbar_On_Empty
Measurements: https://mozilla.invisionapp.com/share/63D3667VZ#/248943707_Customize_-_Measurements

Comment 9

[:Gijs (out for now; he/him)]

Attachment: Bug 1389167 - make bookmarks toolbar more visible in customize mode,

Review commit: https://reviewboard.mozilla.org/r/170142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170142/

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

https://reviewboard.mozilla.org/r/170142/#review175264

::: browser/themes/shared/browser.inc.css:38
(Diff revision 1)
>  
> +#navigator-toolbox > toolbar[customizing]:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar) {
> +  background-color: rgba(12,12,13,0.1);
> +  border-radius: 2px;
> +  margin-left: 4px;
> +  margin-right: 4px;

I expect this won't work as intended when the window is transparent or has some other background that's different from our toolbar background (e.g. aero glass, or even Windows 10 where we currently use -moz-win-glass).

Maybe you can achive a similar effect using an inset box-shadow.

Otherwise I'd go back to my suggestion to use outline.

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170142/diff/1-2/

Comment 12

[:Gijs (out for now; he/him)]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

This drops the border radius and margins as well, because as Dão suggested, on win10 glass and (I expect, only tested win10) on win7 glass, the radius / margin results in odd differently-coloured blobs at the edges. I don't think there's a way to get an inset border that doesn't go all the way to the edge, or that has 4 rounded corners.

Comment 13

[:Gijs (out for now; he/him)]

Trypush with builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a1dd20f5a2f

Comment 14

[Dão Gottwald [:dao]]

I think an outline with a negative offset would be preferable over making the whole toolbar darker without any margin around it...

Comment 15

[Aaron Benson]

(In reply to Dão Gottwald [::dao] from comment #14)
> I think an outline with a negative offset would be preferable over making
> the whole toolbar darker without any margin around it...

Yep, this would work for me too. Gijs, can we make an outline with a color of Grey 30 (#D7D7DB).

Comment 16

[:Gijs (out for now; he/him)]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170142/diff/2-3/

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

https://reviewboard.mozilla.org/r/170142/#review175298

::: browser/themes/shared/browser.inc.css:35
(Diff revision 3)
>    max-height: 0;
>    transition: min-height 170ms ease-out, max-height 170ms ease-out, visibility 170ms linear;
>  }
>  
> +#navigator-toolbox > toolbar[customizing]:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar) {
> +  outline: 1px solid rgba(12,12,13,0.2);

I think this should be dotted or dashed so that the toolbar doesn't look like a big flexible space.

::: browser/themes/shared/browser.inc.css:35
(Diff revision 3)
>    max-height: 0;
>    transition: min-height 170ms ease-out, max-height 170ms ease-out, visibility 170ms linear;
>  }
>  
> +#navigator-toolbox > toolbar[customizing]:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar) {
> +  outline: 1px solid rgba(12,12,13,0.2);

We still support arbitrary Gtk themes on Linux and I'm sure there are dark themes where this color won't be visible. Can you use ThreeDShadow?

Also need to take dark lightweight themes into account, including our own Dark theme.

The easiest solution that works in all scenarios would be to omit the color (equivalent to using currentColor). This would however make the outline stand out more than intended. Making it dotted or dashed should help with that.

::: browser/themes/shared/browser.inc.css:39
(Diff revision 3)
> +#navigator-toolbox > toolbar[customizing]:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar) {
> +  outline: 1px solid rgba(12,12,13,0.2);
> +  outline-offset: -4px;
> +  -moz-outline-radius: 2px;
> +  /* Avoid the toolbar having no height when there's no items in it */
> +  min-height: calc(16px + 2 * var(--toolbarbutton-inner-padding));

bookmark items are narrower and don't use --toolbarbutton-inner-padding, so this min-height would make the toolbar grow unexpectedly in that case. I think this can just be hardcoded to 16px or 18px or so.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [::dao] from comment #17)
> Comment on attachment 8898726 [details]
> Bug 1389167 - make bookmarks toolbar more visible in customize mode,
> 
> https://reviewboard.mozilla.org/r/170142/#review175298
> 
> ::: browser/themes/shared/browser.inc.css:35
> (Diff revision 3)
> >    max-height: 0;
> >    transition: min-height 170ms ease-out, max-height 170ms ease-out, visibility 170ms linear;
> >  }
> >  
> > +#navigator-toolbox > toolbar[customizing]:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar) {
> > +  outline: 1px solid rgba(12,12,13,0.2);
> 
> I think this should be dotted or dashed so that the toolbar doesn't look
> like a big flexible space.
> 
> ::: browser/themes/shared/browser.inc.css:35
> (Diff revision 3)
> >    max-height: 0;
> >    transition: min-height 170ms ease-out, max-height 170ms ease-out, visibility 170ms linear;
> >  }
> >  
> > +#navigator-toolbox > toolbar[customizing]:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar) {
> > +  outline: 1px solid rgba(12,12,13,0.2);
> 
> We still support arbitrary Gtk themes on Linux and I'm sure there are dark
> themes where this color won't be visible. Can you use ThreeDShadow?
> 
> Also need to take dark lightweight themes into account, including our own
> Dark theme.
> 
> The easiest solution that works in all scenarios would be to omit the color
> (equivalent to using currentColor). This would however make the outline
> stand out more than intended. Making it dotted or dashed should help with
> that.

--> Aaron. I asked about both of these things and was told it should be solid, and this colour. Maybe the linux thing makes a difference, I don't know.

> ::: browser/themes/shared/browser.inc.css:39
> (Diff revision 3)
> > +#navigator-toolbox > toolbar[customizing]:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar) {
> > +  outline: 1px solid rgba(12,12,13,0.2);
> > +  outline-offset: -4px;
> > +  -moz-outline-radius: 2px;
> > +  /* Avoid the toolbar having no height when there's no items in it */
> > +  min-height: calc(16px + 2 * var(--toolbarbutton-inner-padding));
> 
> bookmark items are narrower and don't use --toolbarbutton-inner-padding, so
> this min-height would make the toolbar grow unexpectedly in that case. I
> think this can just be hardcoded to 16px or 18px or so.

This is not what I see on Windows 10. With the bookmarks toolbar items in the toolbar, if I change to 'touch' density, the bookmarks toolbar grows taller, and it is 34px tall (16px + 2 * 9px padding). The toolbarbutton placeholder gets the padding from the same relevant rule in browser.css, including --toolbarbutton-inner-padding. What would you like me to do about this?

Comment 19

[Aaron Benson]

Good point about it looking like a flexible spacer. A dashed line would be fine. As for color issues, I'm mostly trying to follow the Photon color guide but if there are other, more appropriate, color values to inherit from then that's fine too.

Comment 20

[Aaron Benson]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

https://reviewboard.mozilla.org/r/170142/#review175282

The dashed line probably works best (otherwise could be confused as a flexible spacer).

Comment 21

[:Gijs (out for now; he/him)]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170142/diff/3-4/

Comment 22

[Aaron Benson]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

https://reviewboard.mozilla.org/r/170142/#review175346

Great!

Comment 23

[:Gijs (out for now; he/him)]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

Clearing ni - one request per bug is enough.

Comment 24

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #18)
> This is not what I see on Windows 10. With the bookmarks toolbar items in
> the toolbar, if I change to 'touch' density, the bookmarks toolbar grows
> taller, and it is 34px tall (16px + 2 * 9px padding). The toolbarbutton
> placeholder gets the padding from the same relevant rule in browser.css,
> including --toolbarbutton-inner-padding. What would you like me to do about
> this?

Do you have the overflow chevron on the bookmarks toolbar? That makes it taller (bug 1389966). Bookmark items adjust to compact / touch mode but not by using --toolbarbutton-inner-padding: http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/browser/themes/shared/toolbarbuttons.inc.css#256-268

Comment 25

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [::dao] from comment #24)
> (In reply to :Gijs from comment #18)
> > This is not what I see on Windows 10. With the bookmarks toolbar items in
> > the toolbar, if I change to 'touch' density, the bookmarks toolbar grows
> > taller, and it is 34px tall (16px + 2 * 9px padding). The toolbarbutton
> > placeholder gets the padding from the same relevant rule in browser.css,
> > including --toolbarbutton-inner-padding. What would you like me to do about
> > this?
> 
> Do you have the overflow chevron on the bookmarks toolbar? That makes it
> taller (bug 1389966). Bookmark items adjust to compact / touch mode but not
> by using --toolbarbutton-inner-padding:
> http://searchfox.org/mozilla-central/rev/
> e8c36327cd8c9432c69e5e1383156a74330f11f2/browser/themes/shared/
> toolbarbuttons.inc.css#256-268

No, I'm talking about within customize mode. Then no bookmark items show up, and no chevron either - just the placeholder button.

Comment 26

[Dão Gottwald [:dao]]

Ah, the placeholder "button" that never really acts as a button. I don't think this thing should be a toolbarbutton-1 in the first place; we should treat it as a bug that this makes the toolbar taller while customizing rather than implementing the same weird behavior a second time here.

Comment 27

[:Gijs (out for now; he/him)]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170142/diff/4-5/

Comment 28

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [::dao] from comment #26)
> Ah, the placeholder "button" that never really acts as a button. I don't
> think this thing should be a toolbarbutton-1 in the first place; we should
> treat it as a bug that this makes the toolbar taller while customizing
> rather than implementing the same weird behavior a second time here.

OK. I went with 22px because although you suggested 16/18px but that looks way too small, esp. given the negative offset of the outline. Let me know if you want me to pick a different value.

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 8898726 [details]
Bug 1389167 - make bookmarks toolbar more visible in customize mode,

https://reviewboard.mozilla.org/r/170142/#review175914

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/490b9614bd85
make bookmarks toolbar more visible in customize mode, r=abenson+572682,dao

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/490b9614bd85

Comment 32

[Saddam Hossain]

I have reproduced this Bug on Nightly 57.0a1 (2017-08-10) on Windows 10, 64 bit!

The bug's fix is now verified on latest  Nightly 57.0a1

Build ID    :	20170829100404
User Agent  : 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 33

[Aaron Benson]

Attachment: bookmark-toolbar-16.svg

Updated icon for this and there should be extra padding around the toolbar item. See the spec for more detail: https://mozilla.invisionapp.com/share/FKD366GDM#/248932284_Customize_-_Toolbar_On