1387117 - Especially in maximized window, it is almost invisible the container color indicator. And it is no longer visible for selected tab

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Alice0775 White]

Attachment: screenshot, the container color indicator should be red color

After landing Bug 1349555,

Actual Results:
Especially in maximized window,
it is almost invisible the container color indicator.
And it is no longer visible for selected tab

Expected Results:
I think it is need more thicker line.
It needs to be easily distinguishable.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Currently working on a fix for this. For the time being you can use the Test Pilot extension which should fix it for the time being.

Comment 2

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1387117 - Fix containers highlight for square tabs to be an underline.

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

Comment 3

[Jonathan Kingston [:jkt] he/him]

Attachment: Selection_759.png

I did the work here as pushing to review wouldn't work for the other bug. I used a hbox in the same way as you did for the line. Attached is a screenshot of a preview on linux.

Comment 4

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cd67eeb7e83d2d0f9e32a93183a6969069d092b

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8893489 [details]
Bug 1387117 - Fix containers highlight for square tabs to be an underline.

https://reviewboard.mozilla.org/r/164562/#review169938

::: browser/base/content/tabbrowser.xml:7387
(Diff revision 1)
>        <xul:stack class="tab-stack" flex="1">
>          <xul:vbox xbl:inherits="selected=visuallyselected,fadein"
>                    class="tab-background">
>            <xul:hbox xbl:inherits="selected=visuallyselected"
>                      class="tab-line"/>
> +          <xul:hbox xbl:inherits="selected=visuallyselected"

Looks like you're not using the selected attribute, so no need to inherit it.

::: browser/base/content/tabbrowser.xml:7387
(Diff revision 1)
>        <xul:stack class="tab-stack" flex="1">
>          <xul:vbox xbl:inherits="selected=visuallyselected,fadein"
>                    class="tab-background">
>            <xul:hbox xbl:inherits="selected=visuallyselected"
>                      class="tab-line"/>
> +          <xul:hbox xbl:inherits="selected=visuallyselected"

Please add a <xul:spacer flex="1"/> between tab-line and this node. This will make the styling simpler (no position:absolute needed).

::: browser/base/content/tabbrowser.xml:7388
(Diff revision 1)
>          <xul:vbox xbl:inherits="selected=visuallyselected,fadein"
>                    class="tab-background">
>            <xul:hbox xbl:inherits="selected=visuallyselected"
>                      class="tab-line"/>
> +          <xul:hbox xbl:inherits="selected=visuallyselected"
> +                    class="container-indicator"/>

Let's name this a little more generically, e.g. tab-bottom-line.

::: browser/components/contextualidentity/content/usercontext.css:77
(Diff revision 1)
>    -moz-box-align: center;
>  }
>  
> -.tabbrowser-tab[usercontextid] {
> -  background-image: linear-gradient(to right, transparent 20%, var(--identity-tab-color) 30%, var(--identity-tab-color) 70%, transparent 80%);
> -  background-size: auto 2px;
> +.container-indicator {
> +  display: none;
> +}

.tabbrowser-tab:not([usercontextid]) > .tab-stack > .tab-background > .container-indicator {
  display: none;
}

Comment 6

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8893489 [details]
Bug 1387117 - Fix containers highlight for square tabs to be an underline.

https://reviewboard.mozilla.org/r/164562/#review169938

> Let's name this a little more generically, e.g. tab-bottom-line.

I don't think there is a nicer solution to relative sizing that position: absolute; I had bz confirm this on irc that it removes it from xul layout and blockifies it allowing the use of %'s in the margin. He pointed me to the algo used:
http://searchfox.org/mozilla-central/source/layout/xul/nsBox.cpp#353-356

Unless you can think of another direction, I think that will need to stay.

Comment 7

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8893489 [details]
Bug 1387117 - Fix containers highlight for square tabs to be an underline.

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

Comment 8

[Jonathan Kingston [:jkt] he/him]

Also :dao is there any 57 theme specific selector I can remove the styles with in our extension? Is :-moz-lwtheme good enough?

I updated the patch, I couldn't do one of the comments, see above.

Thanks

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8893489 [details]
Bug 1387117 - Fix containers highlight for square tabs to be an underline.

https://reviewboard.mozilla.org/r/164562/#review170316

::: browser/components/contextualidentity/content/usercontext.css:86
(Diff revision 2)
> +  bottom: 0;
> +  display: block;
> +  height: 2px;
> +  margin-inline-end: 10%;
> +  margin-inline-start: 10%;
> +  position: absolute;

I think you can use linear-gradient from transparent to var(--identity-tab-color) to transparent as a background image, with percentage values for the color stops.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Jonathan Kingston [:jkt] from comment #8)
> Also :dao is there any 57 theme specific selector I can remove the styles
> with in our extension?

.tab-bottom-line will only exist in 57, so your extension can just hide that, if that's what you want.

> Is :-moz-lwtheme good enough?

No, that's something else entirely.

Comment 13

[Jonathan Kingston [:jkt] he/him]

I might have to use JS then, I want to disable the extensions CSS for 57.

I'm going to change the pinned tab highlight to be fixed pixels based on this:
https://bugzilla.mozilla.org/show_bug.cgi?id=1387413

So would be:
background: linear-gradient(to right, transparent 12px, var(--identity-tab-color) 12px, var(--identity-tab-color) 28px, transparent 28px);

Waiting for a new compile to test.

Comment 14

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8893489 [details]
Bug 1387117 - Fix containers highlight for square tabs to be an underline.

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

Comment 15

[Jonathan Kingston [:jkt] he/him]

Attachment: Latest patch preview in ubuntu

Comment 16

[Jonathan Kingston [:jkt] he/him]

Should be good now, this depends on your pinned tabs patch as they are fixed width I made this account for those dimensions.
Thanks

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8893489 [details]
Bug 1387117 - Fix containers highlight for square tabs to be an underline.

https://reviewboard.mozilla.org/r/164562/#review170446

::: browser/components/contextualidentity/content/usercontext.css:76
(Diff revision 3)
>  #userContext-icons {
>    -moz-box-align: center;
>  }
>  
> -.tabbrowser-tab[usercontextid] {
> -  background-image: linear-gradient(to right, transparent 20%, var(--identity-tab-color) 30%, var(--identity-tab-color) 70%, transparent 80%);
> +.tabbrowser-tab:not([usercontextid]) > .tab-stack > .tab-background > .tab-bottom-line {
> +  display: none;

There's no need to do this. The node is invisible when unstyled.

::: browser/components/contextualidentity/content/usercontext.css:87
(Diff revision 3)
> +  height: 2px;
> +}
> +
> +.tabbrowser-tab[pinned][usercontextid] > .tab-stack > .tab-background > .tab-bottom-line {
> +  /* from .tab-content padding and icon width */
> +  background: linear-gradient(to right, transparent 12px, var(--identity-tab-color) 12px, var(--identity-tab-color) 28px, transparent 28px);

Can you use percentages here? E.g. 12px should be 30%. Makes this more readable without knowing the exact tab width.

Comment 18

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8893489 [details]
Bug 1387117 - Fix containers highlight for square tabs to be an underline.

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

Comment 19

[Jonathan Kingston [:jkt] he/him]

> There's no need to do this. The node is invisible when unstyled.
Isn't that what you were asking for in Comment 5? Removed it.

> Can you use percentages here? E.g. 12px should be 30%. Makes this more readable without knowing the exact tab width.

Sure I removed the comment too.

I'd like to make a follow up to move the attention image "indicator-tab-attention.svg" to be on the tab-background such that the bottom line can sit on top of this. (we have had complaints before that this indicator makes the pinned tabs impossible to see the container).

Comment 20

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1387117 - Move tab attention indicator to background hbox so containers indicator is overlaid.

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

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8893489 [details]
Bug 1387117 - Fix containers highlight for square tabs to be an underline.

https://reviewboard.mozilla.org/r/164562/#review170514

::: browser/components/contextualidentity/content/usercontext.css:77
(Diff revision 4)
>    -moz-box-align: center;
>  }
>  
> -.tabbrowser-tab[usercontextid] {
> -  background-image: linear-gradient(to right, transparent 20%, var(--identity-tab-color) 30%, var(--identity-tab-color) 70%, transparent 80%);
> -  background-size: auto 2px;
> +.tabbrowser-tab[usercontextid] > .tab-stack > .tab-background > .tab-bottom-line {
> +  background: linear-gradient(to right, transparent 10%, var(--identity-tab-color) 10%, var(--identity-tab-color) 90%, transparent 90%);
> +  display: block;

Please remove this line.

Comment 22

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8893489 [details]
Bug 1387117 - Fix containers highlight for square tabs to be an underline.

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

Comment 23

[Jonathan Kingston [:jkt] he/him]

Thanks :dao have filed Bug 1387693 for a follow up on the pinned tab indicator.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 25

[Jonathan Kingston [:jkt] he/him]

I'm really sorry :RyanVM I never learn :(. Fixed those issues now.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ae81136b681
Fix containers highlight for square tabs to be an underline. r=dao

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4ae81136b681