Closed Bug 1387117 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Theme, defect)

57 Branch
Unspecified
Windows 10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: alice0775, Assigned: jkt)

References

Details

(Keywords: regression, Whiteboard: [userContextId])

Attachments

(4 files, 1 obsolete file)

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.
Whiteboard: [photon-visual] [triage]
Component: Tabbed Browser → Theme
See Also: → 1325902
Whiteboard: [photon-visual] [triage] → [userContextId]
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.
Assignee: nobody → jkt
Attached image 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.
Flags: needinfo?(dao+bmo)
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;
}
Attachment #8893489 - Flags: review?(dao+bmo) → review-
Flags: needinfo?(dao+bmo)
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.
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
Flags: needinfo?(dao+bmo)
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.
Attachment #8893489 - Flags: review?(dao+bmo) → review-
(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.
Flags: needinfo?(dao+bmo)
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.
Depends on: 1387413
Should be good now, this depends on your pinned tabs patch as they are fixed width I made this account for those dimensions.
Thanks
Flags: needinfo?(dao+bmo)
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.
Attachment #8893489 - Flags: review?(dao+bmo) → review-
> 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 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.
Attachment #8893489 - Flags: review?(dao+bmo) → review+
Attachment #8894012 - Attachment is obsolete: true
Attachment #8894012 - Flags: review?(dao+bmo)
Blocks: 1387693
Thanks :dao have filed Bug 1387693 for a follow up on the pinned tab indicator.
Flags: needinfo?(dao+bmo)
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(jkt)
Keywords: checkin-needed
I'm really sorry :RyanVM I never learn :(. Fixed those issues now.
Keywords: checkin-needed
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ae81136b681
Fix containers highlight for square tabs to be an underline. r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ae81136b681
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.