Brian Grinstead [:bgrins] Reporter
	Description • 10 years ago

See this screenshot: http://cl.ly/image/3Q3m1m122E0y/o from here: https://bugzilla.mozilla.org/show_bug.cgi?id=1093820#c3

	Brian Grinstead [:bgrins] Reporter
	Comment 1 • 10 years ago

WIP only - just putting this here so I don't forget.  This seems a little tricky with back/forward button because the back button usually doesn't have a border applied.

	Tim Nguyen :ntim Assignee
	Comment 2 • 10 years ago

(In reply to Brian Grinstead [:bgrins] from comment #0)
> See this screenshot: http://cl.ly/image/3Q3m1m122E0y/o from here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1093820#c3

In dark theme, it looks more like a box-shadow than a border.

	Jordan Santell [:jsantell] [@jsantell]
	Comment 3 • 10 years ago

Pushed brian's patch and some windows styling to try so i can test builds on other OS: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=969f159bb4a8

	Jordan Santell [:jsantell] [@jsantell]
	Comment 4 • 10 years ago

Do we have mockups for dev edition where both back and forward buttons are shown? Should there be a border between these nav buttons?

	Brian Grinstead [:bgrins] Reporter
	Comment 5 • 10 years ago

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> Do we have mockups for dev edition where both back and forward buttons are
> shown? Should there be a border between these nav buttons?

Talked with Stephen - yes there should be a border between these buttons.

	Jordan Santell [:jsantell] [@jsantell]
	Comment 6 • 10 years ago

Great, thanks Brian!

Matteo, passing this to you as its more your thing -- can you take this?

	Tim Nguyen :ntim Assignee
	Comment 7 • 10 years ago

Made the WIP patch work on Windows + fixed some oddities in light theme.

	Brian Grinstead [:bgrins] Reporter
	Comment 8 • 10 years ago

(In reply to Tim Nguyen [:ntim] from comment #7)
> Created attachment 8532820 [details] [diff] [review]
> devedition-borders-WIP.patch
> 
> Made the WIP patch work on Windows + fixed some oddities in light theme.

Thanks!

	Tim Nguyen :ntim Assignee
	Comment 9 • 10 years ago

Comment on attachment 8532820 [details] [diff] [review]
devedition-borders-WIP.patch

Needs rebasing, and some extra fixes for the findbar.

	Tim Nguyen :ntim Assignee
	Comment 11 • 10 years ago

Tiny change

	:Gijs (he/him)
	Comment 12 • 10 years ago

Comment on attachment 8546889 [details] [diff] [review]
Patch v0.2

Review of attachment 8546889 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/devedition.inc.css
@@ +100,5 @@
>    --pinned-tab-glow: radial-gradient(22px at center calc(100% - 2px), rgba(76,158,217,0.9) 13%, transparent 16%);
>  
>  
>    /* Toolbar buttons */
> +  --toolbarbutton-hover-background: #fcfcfc;

Why does this bug involve changing all the light colors?

@@ +201,5 @@
>    color: var(--chrome-color) !important; /* Make sure that the brighttext attribute is added */
>  }
>  
>  /* URL bar and search bar*/
>  /* XXX :root[devtoolstheme="dark"] is a workaround for bug 1096413 on the findbar. */

Why did you remove this, and not the comment? Either keep the selector, or remove both and explain why that is OK, please.

	Tim Nguyen :ntim Assignee
	Comment 13 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 8546889 [details] [diff] [review]
> Patch v0.2
> 
> Review of attachment 8546889 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/devedition.inc.css
> @@ +100,5 @@
> >    --pinned-tab-glow: radial-gradient(22px at center calc(100% - 2px), rgba(76,158,217,0.9) 13%, transparent 16%);
> >  
> >  
> >    /* Toolbar buttons */
> > +  --toolbarbutton-hover-background: #fcfcfc;
> 
> Why does this bug involve changing all the light colors?
The active state was ligher than the hover state, which was pretty strange. But I can always handle that later.

> @@ +201,5 @@
> >    color: var(--chrome-color) !important; /* Make sure that the brighttext attribute is added */
> >  }
> >  
> >  /* URL bar and search bar*/
> >  /* XXX :root[devtoolstheme="dark"] is a workaround for bug 1096413 on the findbar. */
> 
> Why did you remove this, and not the comment? Either keep the selector, or
> remove both and explain why that is OK, please.
Whoops, forgot to remove that.

	Tim Nguyen :ntim Assignee
	Comment 14 • 10 years ago

Removed the comment.

:root[devtoolstheme="dark"] was a hack for this bug, on the find bar. It would have removed the border of the findbar text box on light theme, if I didn't add that (the dark theme was ok, since the contrast was significant enough to not need a border), now that the textbox related rules include border, :root[devtoolstheme="dark"] is no longer necessary.

About the light theme color changes, I made the toolbar background and the urlbar controls background match the latest mockups from shorlander :
I also fixed the fact that the toolbar button active state was lighter than the toolbar button hover state, which was pretty weird, I used the back button background color for the hover state, and the old hover background color for the active state.

	:Gijs (he/him)
	Comment 15 • 9 years ago

Comment on attachment 8547202 [details] [diff] [review]
Patch v0.3

Review of attachment 8547202 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks fine, but...

	:Gijs (he/him)
	Comment 16 • 9 years ago

There's a gap between the back/fwd button border and the navbar border (probably because of border-radius + border-end: none. You might need 0 none (ie fix the width) and/or to reset the relevant border radius bits?

	:Gijs (he/him)
	Comment 17 • 9 years ago

The end-side border of the findbar textfield also makes the up/down arrow buttons look odd.

	Tim Nguyen :ntim Assignee
	Comment 18 • 9 years ago

This fixes both issues, here's what I changed :
- The border gap was caused since the forward button was too wide (it doesn't have a border on normal theme, but has a box-shadow). So the fix here is to remove the left border of the forward button when it's disabled (invisible), so it takes less space. One other solution was to increase the negative left margin of the forward button by 1px. But I found the first solution to be better, since it's unlikely going to break in case we change something in the conditional forward button code.

- For the findbar, it now only changes the textbox background and color, and leaves the border-color with the old style (and matching the previous and next button border-color).

	:Gijs (he/him)
	Comment 19 • 9 years ago

Comment on attachment 8547777 [details] [diff] [review]
Patch v1

Review of attachment 8547777 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies for the delay here. I don't think your fix for the back/fwd button is the right one. Now, after going back in a way that disables the forward button, the back button has no end-side border (on Windows, anyway). I'll attach a screenshot in a bit. It looks jarring while the animation has not finished yet.

Seems to me like we should go with the other option and pull the navbar forward an extra pixel so the borders overlap. I'm not sure what kind of change in the back/fwd code would break that solution...

Your OS X hunks also miss the removal of the start border... what's different there? Did you test on OS X or should I?

::: browser/themes/shared/devedition.inc.css
@@ +219,5 @@
>  
> +/* Findbar textbox */
> +.browserContainer > findbar .findbar-textbox {
> +  background-color: var(--url-and-searchbar-background-color) !important;
> +  color: var(--url-and-searchbar-color);

FWIW, please file a follow-up bug for the buttons next to the find field in the dark theme. They look awful.

	Tim Nguyen :ntim Assignee
	Comment 21 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #19)
> Comment on attachment 8547777 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8547777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Apologies for the delay here. I don't think your fix for the back/fwd button
> is the right one. Now, after going back in a way that disables the forward
> button, the back button has no end-side border (on Windows, anyway). I'll
> attach a screenshot in a bit. It looks jarring while the animation has not
> finished yet.
I can try removing the #forward-button end border instead of removing it's start border.

> Seems to me like we should go with the other option and pull the navbar
> forward an extra pixel so the borders overlap. I'm not sure what kind of
> change in the back/fwd code would break that solution...
I don't think that's future proof, if calculations get changed in the future.

> Your OS X hunks also miss the removal of the start border... what's
> different there? Did you test on OS X or should I?
Brian said OSX didn't have this issue. It's likely because OSX uses a border instead of a box-shadow (for the back button) for the default theme. DevEdition uses a border for all platforms.

> ::: browser/themes/shared/devedition.inc.css
> @@ +219,5 @@
> >  
> > +/* Findbar textbox */
> > +.browserContainer > findbar .findbar-textbox {
> > +  background-color: var(--url-and-searchbar-background-color) !important;
> > +  color: var(--url-and-searchbar-color);
> 
> FWIW, please file a follow-up bug for the buttons next to the find field in
> the dark theme. They look awful.
Filed bug 1121432.

	Tim Nguyen :ntim Assignee
	Comment 22 • 9 years ago

Similar hack, but not affecting the forward button animation.

	:Gijs (he/him)
	Comment 23 • 9 years ago

Comment on attachment 8552022 [details] [diff] [review]
Patch v1.1

Review of attachment 8552022 [details] [diff] [review]:
-----------------------------------------------------------------

This has bitrotted significantly.

This also (permanently) leaves no border at all between the back and forward buttons on OS X when both are enabled.

::: browser/themes/shared/devedition.inc.css
@@ +100,5 @@
>    --pinned-tab-glow: radial-gradient(22px at center calc(100% - 2px), rgba(76,158,217,0.9) 13%, transparent 16%);
>  
>  
>    /* Toolbar buttons */
> +  --toolbarbutton-hover-background: #fcfcfc;

This means the hover color is lighter than the toolbar's background color, but the active state is darker again. That seems wrong... where is the spec that says we should be doing this? Why not just do what we do on win8 (very similar toolbar color, too) and use slightly darker hover, and still darker active colors?

	Tim Nguyen :ntim Assignee
	Comment 24 • 9 years ago

Looks great on OSX now (at least on Yosemite).
Also made the hover state darker than normal state.

	:Gijs (he/him)
	Comment 26 • 9 years ago

Comment on attachment 8588411 [details] [diff] [review]
Patch v2.1

Review of attachment 8588411 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still seeing the back button's end border disappear during the animation of the forward button (both in and out). Can we fix that?

::: browser/themes/linux/devedition.css
@@ +27,5 @@
>  }
>  
>  /* Square back and forward buttons */
>  #back-button:not(:-moz-lwtheme) > .toolbarbutton-icon,
>  #forward-button:not(:-moz-lwtheme) > .toolbarbutton-icon {

Pretty sure this rule is dead now, because we'll always be using a lwtheme... :-\

::: browser/themes/osx/devedition.css
@@ +45,3 @@
>    box-shadow: none !important;
> +  border: 1px solid var(--chrome-nav-bar-controls-border-color) !important;
> +  -moz-border-end: none !important;

In fact, seeing as you're doing this everywhere, seems like that should be pulled into devedition.inc.css ?

Instead of this, which has this animation drawback, can we make the back-button have both start and end borders, and the forward button just an end border, and the url bar no start border? Is there some reason that doesn't work?

::: browser/themes/shared/devedition.inc.css
@@ +109,2 @@
>    --toolbarbutton-active-boxshadow: none;
> +  --toolbarbutton-active-bordercolor: rgba(0,0,0,0.1);

Why change this?

@@ +109,3 @@
>    --toolbarbutton-active-boxshadow: none;
> +  --toolbarbutton-active-bordercolor: rgba(0,0,0,0.1);
> +  --toolbarbutton-checkedhover-backgroundcolor: #d7d7d8;

I'm nervous about changing this to be opaque. Why was this necessary?

	:Gijs (he/him)
	Comment 27 • 9 years ago

(also, sorry about the delay here - I was busy over the (long holiday) weekend, and have been struggling to catch up with my queue)

	Brian Grinstead [:bgrins] Reporter
	Comment 28 • 9 years ago

Comment on attachment 8588411 [details] [diff] [review]
Patch v2.1

Review of attachment 8588411 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/linux/devedition.css
@@ +27,5 @@
>  }
>  
>  /* Square back and forward buttons */
>  #back-button:not(:-moz-lwtheme) > .toolbarbutton-icon,
>  #forward-button:not(:-moz-lwtheme) > .toolbarbutton-icon {

Yeah, looks like I missed that rule in the conversion.  Just confirmed locally that simply removing the :not(:-moz-lwtheme) from the selector fixes the problem.  I'll file a quick separate bug to fix this

	Brian Grinstead [:bgrins] Reporter
	Comment 29 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #28)
> Comment on attachment 8588411 [details] [diff] [review]
> Patch v2.1
> 
> Review of attachment 8588411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/linux/devedition.css
> @@ +27,5 @@
> >  }
> >  
> >  /* Square back and forward buttons */
> >  #back-button:not(:-moz-lwtheme) > .toolbarbutton-icon,
> >  #forward-button:not(:-moz-lwtheme) > .toolbarbutton-icon {
> 
> Yeah, looks like I missed that rule in the conversion.  Just confirmed
> locally that simply removing the :not(:-moz-lwtheme) from the selector fixes
> the problem.  I'll file a quick separate bug to fix this

Tim, don't worry about changing the :not(:-moz-lwtheme) in the selectors here, I've fixed this in fxteam in Bug 1096413.

	Tim Nguyen :ntim Assignee
	Comment 30 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #26)
> Comment on attachment 8588411 [details] [diff] [review]
> Patch v2.1
> 
> Review of attachment 8588411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm still seeing the back button's end border disappear during the animation
> of the forward button (both in and out). Can we fix that?
I'll try to fix that. It's pretty hard to notice though (at least on OSX).

> ::: browser/themes/osx/devedition.css
> @@ +45,3 @@
> >    box-shadow: none !important;
> > +  border: 1px solid var(--chrome-nav-bar-controls-border-color) !important;
> > +  -moz-border-end: none !important;
> 
> In fact, seeing as you're doing this everywhere, seems like that should be
> pulled into devedition.inc.css ?
Nope, the rule applies to .toolbarbutton-icon on Win and Linux, while it applies on the toolbarbutton itself on OSX.

> Instead of this, which has this animation drawback, can we make the
> back-button have both start and end borders, and the forward button just an
> end border, and the url bar no start border? Is there some reason that
> doesn't work?
Will try that.

> ::: browser/themes/shared/devedition.inc.css
> @@ +109,2 @@
> >    --toolbarbutton-active-boxshadow: none;
> > +  --toolbarbutton-active-bordercolor: rgba(0,0,0,0.1);
> 
> Why change this?
It must be a leftover from the previous patch, I probably changed it since it looked nicer to me. I'll restore the old border color.

> @@ +109,3 @@
> >    --toolbarbutton-active-boxshadow: none;
> > +  --toolbarbutton-active-bordercolor: rgba(0,0,0,0.1);
> > +  --toolbarbutton-checkedhover-backgroundcolor: #d7d7d8;
> 
> I'm nervous about changing this to be opaque. Why was this necessary?
The hover state was already opaque, and the active state was broken because of a missing semicolon. So I decided to make everything consistent by using opaque colors for all states.

(In reply to Brian Grinstead [:bgrins] from comment #29)
> (In reply to Brian Grinstead [:bgrins] from comment #28)
> > Comment on attachment 8588411 [details] [diff] [review]
> > Patch v2.1
> > 
> > Review of attachment 8588411 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/themes/linux/devedition.css
> > @@ +27,5 @@
> > >  }
> > >  
> > >  /* Square back and forward buttons */
> > >  #back-button:not(:-moz-lwtheme) > .toolbarbutton-icon,
> > >  #forward-button:not(:-moz-lwtheme) > .toolbarbutton-icon {
> > 
> > Yeah, looks like I missed that rule in the conversion.  Just confirmed
> > locally that simply removing the :not(:-moz-lwtheme) from the selector fixes
> > the problem.  I'll file a quick separate bug to fix this
> 
> Tim, don't worry about changing the :not(:-moz-lwtheme) in the selectors
> here, I've fixed this in fxteam in Bug 1096413.
Thanks Brian !

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 31 • 9 years ago

Setting devedition-40 bugs to P2, filter on C17C996C-A0BE-4153-8057-FAB642D0746D

	Tim Nguyen :ntim Assignee
	Comment 32 • 9 years ago

Took the approach you suggested. Works fine on OSX, still need to test it on Windows and Linux.

	Tim Nguyen :ntim Assignee
	Comment 33 • 9 years ago

(In reply to Tim Nguyen [:ntim] from comment #30)
> (In reply to :Gijs Kruitbosch from comment #26)
> > ::: browser/themes/shared/devedition.inc.css
> > @@ +109,2 @@
> > >    --toolbarbutton-active-boxshadow: none;
> > > +  --toolbarbutton-active-bordercolor: rgba(0,0,0,0.1);
> > 
> > Why change this?
> It must be a leftover from the previous patch, I probably changed it since
> it looked nicer to me. I'll restore the old border color.
Changed this into a slightly darker color, but I didn't restore the old one, since it looked too dark.

	:Gijs (he/him)
	Comment 34 • 9 years ago

Comment on attachment 8593548 [details] [diff] [review]
Patch v3

Review of attachment 8593548 [details] [diff] [review]:
-----------------------------------------------------------------

I'm confused... on OS X, I see no internal borders between the two buttons, nor between the back and/or fwd button and the URL bar at all? Just a border to the left of the back button. I thought:

> "can we make the back-button have both start and end borders, and the forward button just an end border, and the url bar no start border"

would mean basically the same outcome as before but *with* borders during the animation. Does this make sense? Maybe I'm not very clear...

I also zoomed in (because on the dark theme the outer borders are hard to spot anyway so I wondered if it was my eyes deceiving me...) and noticed that it seems like the URL bar has a lighter border than the back button/forward button. Is that intentional?

I also noticed from looking at the patch that Linux is currently missing the  -moz-border-end: none thing Windows has?

(In reply to Tim Nguyen [:ntim] from comment #33)
> (In reply to Tim Nguyen [:ntim] from comment #30)
> > (In reply to :Gijs Kruitbosch from comment #26)
> > > ::: browser/themes/shared/devedition.inc.css
> > > @@ +109,2 @@
> > > >    --toolbarbutton-active-boxshadow: none;
> > > > +  --toolbarbutton-active-bordercolor: rgba(0,0,0,0.1);
> > > 
> > > Why change this?
> > It must be a leftover from the previous patch, I probably changed it since
> > it looked nicer to me. I'll restore the old border color.
> Changed this into a slightly darker color, but I didn't restore the old one,
> since it looked too dark.

Looks like this is still the same? Is this the right version of the patch?

::: browser/themes/shared/devedition.inc.css
@@ +228,3 @@
>    box-shadow: none !important;
>  }
> +  

Nit: trailing whitespace.

	Tim Nguyen :ntim Assignee
	Comment 35 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #34)
> Comment on attachment 8593548 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8593548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm confused... on OS X, I see no internal borders between the two buttons,
> nor between the back and/or fwd button and the URL bar at all? Just a border
> to the left of the back button. I thought:
> 
> > "can we make the back-button have both start and end borders, and the forward button just an end border, and the url bar no start border"
> 
> would mean basically the same outcome as before but *with* borders during
> the animation. Does this make sense? Maybe I'm not very clear...
> 
> I also zoomed in (because on the dark theme the outer borders are hard to
> spot anyway so I wondered if it was my eyes deceiving me...) and noticed
> that it seems like the URL bar has a lighter border than the back
> button/forward button. Is that intentional?
> 
> I also noticed from looking at the patch that Linux is currently missing the
> -moz-border-end: none thing Windows has?
Oh, sorry, I made some extra changes on top of this patch, and forgot to refresh them.
I'll upload a new one in a sec.

	Tim Nguyen :ntim Assignee
	Comment 36 • 9 years ago

Here's the properly qref'ed patch

	:Gijs (he/him)
	Comment 37 • 9 years ago

Comment on attachment 8593576 [details] [diff] [review]
Patch v3.1 (properly qref'ed)

Review of attachment 8593576 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! I'm glad I wasn't crazy. This is very nice on OSX, at least. Some minor issues below.

It seems like on Windows (8.1) (and Linux, maybe? I think it's better there, though) the alignment with the urlbar is a little lost and so there's a gap between the back button and the url bar. That needs a little massaging still, not entirely sure what's causing it... For this, f+, but this is getting close - sorry for the repeated false starts!

::: browser/themes/shared/devedition.inc.css
@@ +88,2 @@
>    --chrome-nav-buttons-hover-background: #DADBDB;
> +  --chrome-nav-bar-controls-border-color: #ccc;

So it seems that the discrepancy I'm seeing with colors here is because the urlbar itself is 0.9 opacity because of a rule in browser.css for lightweight themes, which now applies to devedition.

It seems like we should just override that back to 1 for devedition. That will unify the borders, though we may then need to soften the background color a bit further.

@@ +188,5 @@
>  .browserContainer > findbar {
>    background-image: none;
>  }
>  
> +.browserContainer > findbar .findbar-textbox {

Seems like this could be just:

.browserContainer .findbar-textbox

right? Or does that lose a specificity fight with some other rule somewhere?

@@ +228,3 @@
>    box-shadow: none !important;
>  }
> +  

Still some trailing whitespace here...

	Tim Nguyen :ntim Assignee
	Comment 38 • 9 years ago

Addresses all issues.

	:Gijs (he/him)
	Comment 39 • 9 years ago

Comment on attachment 8594146 [details] [diff] [review]
Patch v4

Review of attachment 8594146 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, sweet!

Can you either fix here, or file a followup bug for, the fact that on win8, the -start border corners on the back button are rounded (all the other corners are square, in keeping with win8-ness) ?

	Tim Nguyen :ntim Assignee
	Comment 40 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #39)
> Comment on attachment 8594146 [details] [diff] [review]
> Patch v4
> 
> Review of attachment 8594146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, sweet!
> 
> Can you either fix here, or file a followup bug for, the fact that on win8,
> the -start border corners on the back button are rounded (all the other
> corners are square, in keeping with win8-ness) ?
Thanks for the reviews !
Filed bug 1155958 for the issue.

	:Gijs (he/him)
	Comment 41 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/19225f260c0f

	Phil Ringnalda (:philor)
	Comment 42 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/19225f260c0f