See gif.

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?

(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.

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 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 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?

Done!

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?

(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?

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

(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 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.