947839 - The top green border of a "highlighted" tab doesn't extend over the margins like "selected" tabs

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Victor Porof [:vporof][:vp]]

Attachment: screencast.gif

See gif.

Comment 1

[Albert Juhé]

I'm not sure which is the best solution for this bug. The problem here is that the selected tab has no separators and the highlighted tab does. The selected tab covers the space of the left separator but not the right one as shown here (https://bug941579.bugzilla.mozilla.org/attachment.cgi?id=8338721) this makes the blue border to have one more pixel than the green border.

If we make the active tab not to cover the left separator space, the combination highlighted tab + active tab will look odd because there will be 1px between both tabs (the position where the separator was).

A solution would be to remove the separators also for the highlighted tab or to replace the highlighted tab border with a background image to fake the green box-shadow over the separator. What do you think?

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #1)
> I'm not sure which is the best solution for this bug. The problem here is
> that the selected tab has no separators and the highlighted tab does. The
> selected tab covers the space of the left separator but not the right one as
> shown here
> (https://bug941579.bugzilla.mozilla.org/attachment.cgi?id=8338721) this
> makes the blue border to have one more pixel than the green border.
> 
> If we make the active tab not to cover the left separator space, the
> combination highlighted tab + active tab will look odd because there will be
> 1px between both tabs (the position where the separator was).
> 
> A solution would be to remove the separators also for the highlighted tab or
> to replace the highlighted tab border with a background image to fake the
> green box-shadow over the separator. What do you think?

My initial feeling is to remove the separators for the highlighted tab just as we do for active.  This is consistent design-wise with what we are doing on the active tab and will allow the shadow to extend to the edge without an image.  In fact, I think it may actually look weird to have the green extend over the grey borders.

Comment 3

[Albert Juhé]

Attachment: patch1.patch

I attached a patch that removes the separators for the highlighted tab. I assumed the highlighted tab will never be the first or the last one.

Comment 4

[Albert Juhé]

Attachment: patch1screenshot.png

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8346698 [details] [diff] [review]
patch1.patch

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

Yes, this looks right to me.  I think we should go ahead and plan for the case where this is the first or last tab.  It isn't very likely that the first or last tool will be highlighted but there could be an extension that adds a new last tab, we could reorder the default tool ordering, or we could have the network panel eventually add a highlighted state.  Either way, I'm pretty sure it is just a small change to support this.

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +548,2 @@
>  .devtools-tab[selected=true]:not(:first-child) {
>    -moz-padding-start: 1px;

I suggest we handle the case where this would be the first or last tab - adding the selector .devtools-tab[selected=true]:not(:first-child) to this rule.  Then you can also remove the -moz-padding-start that was added to the .highligted tab below.

@@ +548,5 @@
>  .devtools-tab[selected=true]:not(:first-child) {
>    -moz-padding-start: 1px;
>  }
>  
>  .devtools-tab[selected=true]:last-child {

I suggest we handle the case where this would be the first or last tab - adding the selector .devtools-tab.highlighted:last-child to this rule

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8346698 [details] [diff] [review]
patch1.patch

Victor,
This looks good by my eye, but do you care to have a look at the UI with this patch applied and see if it resolves the reported issue?

Comment 7

[Albert Juhé]

Attachment: patch2.patch

Done!

Comment 8

[Victor Porof [:vporof][:vp]]

Comment on attachment 8346726 [details] [diff] [review]
patch2.patch

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

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +544,5 @@
>                0 -2px 0 rgba(0,0,0,.2) inset;
>  }
>  
> +.devtools-tab[selected=true]:not(:first-child),
> +.devtools-tab.highlighted:not(:first-child) {

Can you please switch from the .highlighted class to a [highlighted] attribute instead, to mirror the "selected" attribute?

Comment 9

[Albert Juhé]

(In reply to Victor Porof [:vp] from comment #8)
> Comment on attachment 8346726 [details] [diff] [review]
> patch2.patch
> 
> Review of attachment 8346726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/devtools/toolbars.inc.css
> @@ +544,5 @@
> >                0 -2px 0 rgba(0,0,0,.2) inset;
> >  }
> >  
> > +.devtools-tab[selected=true]:not(:first-child),
> > +.devtools-tab.highlighted:not(:first-child) {
> 
> Can you please switch from the .highlighted class to a [highlighted]
> attribute instead, to mirror the "selected" attribute?

I will need some help here. I suppose I will have to modify another file, isn't it?

Comment 10

[Victor Porof [:vporof][:vp]]

(fwiw, I wouldn't necessarily block landing on this; can be done in a followup)

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #9)
> (In reply to Victor Porof [:vp] from comment #8)
> > Comment on attachment 8346726 [details] [diff] [review]
> > patch2.patch
> > 
> > Review of attachment 8346726 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/themes/shared/devtools/toolbars.inc.css
> > @@ +544,5 @@
> > >                0 -2px 0 rgba(0,0,0,.2) inset;
> > >  }
> > >  
> > > +.devtools-tab[selected=true]:not(:first-child),
> > > +.devtools-tab.highlighted:not(:first-child) {
> > 
> > Can you please switch from the .highlighted class to a [highlighted]
> > attribute instead, to mirror the "selected" attribute?
> 
> I will need some help here. I suppose I will have to modify another file,
> isn't it?

Yes, we will modify the JavaScript that is adding this class.  Let's open a new bug and work on it there.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8346726 [details] [diff] [review]
patch2.patch

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

Looks good to me.  You can mark checkin-needed, and we'll open up a new bug for addressing the feedback regarding class name versus attributes.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/840fb1599995

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/840fb1599995