Closed Bug 1390025 Opened 3 years ago Closed 3 years ago

Leftmost tab missing its left border in maximized window

Categories

(Firefox :: Theme, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: pharaozh, Assigned: dao)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Keywords: polish, regression, Whiteboard: [reserve-photon-visual])

Attachments

(5 files)

Attached image Missing left tab border
The left tab border of the leftmost tab disappears only when the window is maximized. It reappears when the window is resized smaller. This occurs on the light theme of Photon on macOS.
OS: Unspecified → Mac OS X
Summary: Left tab border missing when maximized → Left tab border on selected tab missing when maximized (light theme, macOS)
Whiteboard: [photon-visual][triage]
I see this on Linux too if there is an item to the left of the tab.
Attached image linux private window
or in private browsing, for that matter.
Blocks: photon-tabs
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: qe-verify+
Priority: -- → P3
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
QA Contact: ovidiu.boca
Happens on Windows too if you pin something to the tab bar before the first tab.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Priority: P3 → P1
Iteration: --- → 57.2 - Aug 29
Duplicate of this bug: 1393807
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Summary: Left tab border on selected tab missing when maximized (light theme, macOS) → Leftmost tab missing its left border in maximized window
Duplicate of this bug: 1396367
Duplicate of this bug: 1395130
Duplicate of this bug: 1399571
Hey Marco, P1, 57 regression, owned, but didn't make it before the merge. Is the 3rd iteration (marked as Sept 19th) finishing up this week or did it end last week? We have a few of these bugs that I need to update 57 status for.
Flags: needinfo?(mmucci)
Keywords: polish
Hi Jim, we are updating our flags on Thursday when we finish the 58 release planning.
Flags: needinfo?(mmucci)
Iteration: 57.3 - Sep 19 → ---
Attachment #8911126 - Flags: review?(jhofmann)
Comment on attachment 8911126 [details]
Bug 1390025 - Add separator between the tab strip and the drag space in front of it.

https://reviewboard.mozilla.org/r/182622/#review187958

As a note: technically the spec requires us to show a border-inline-start on the first tab in overflow, but that doesn't (shouldn't?) have to be part of this patch. We should still file a new bug for it.

::: browser/themes/shared/tabs.inc.css:442
(Diff revision 1)
>    background-color: var(--toolbar-bgcolor);
>    background-image: var(--toolbar-bgimage);
>    background-repeat: repeat-x;
>  }
>  
> -:root:not([sizemode=normal]) .tabbrowser-tab[first-visible-tab] > .tab-stack > .tab-background[selected=true] {
> +%ifdef CAN_DRAW_IN_TITLEBAR

Can you add a comment what this is doing (the whole block)?

::: browser/themes/shared/tabs.inc.css:538
(Diff revision 1)
> +%else
> +:root[tabsintitlebar][sizemode=normal] #TabsToolbar > .titlebar-placeholder[type="pre-tabs"]
> +%endif
> +{
> +  width: 40px;
> +  border-inline-end: 1px solid;

This should not be set on tab overflow, according to the spec (and I agree it looks strange with the border).
Attachment #8911126 - Flags: review?(jhofmann) → review-
(In reply to Johann Hofmann [:johannh] from comment #15)
> This should not be set on tab overflow, according to the spec (and I agree
> it looks strange with the border).

I think the border should be there regardless of overflow to separate the drag space. Stephen?
Flags: needinfo?(shorlander)
(In reply to Dão Gottwald [::dao] from comment #16)
> (In reply to Johann Hofmann [:johannh] from comment #15)
> > This should not be set on tab overflow, according to the spec (and I agree
> > it looks strange with the border).
> 
> I think the border should be there regardless of overflow to separate the
> drag space. Stephen?

Yes, I agree it should be there even for overflow.
Flags: needinfo?(shorlander)
Comment on attachment 8911126 [details]
Bug 1390025 - Add separator between the tab strip and the drag space in front of it.

https://reviewboard.mozilla.org/r/182622/#review188674

Ok, this works for me then, with the nit and follow-up addressed.
Attachment #8911126 - Flags: review- → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0bae5cad94bf
Add separator between the tab strip and the drag space in front of it. r=johannh
https://hg.mozilla.org/mozilla-central/rev/0bae5cad94bf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Improvements noticed:

== Change summary for alert #9663 (as of September 26 2017 13:28 UTC) ==

Improvements:

  3%  tresize windows7-32 opt e10s     12.22 -> 11.90
  2%  tresize windows7-32 pgo e10s     10.72 -> 10.46

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9663
Looking at screenshots in earlier comments of the bug, it looks like separator between pinned tabs used to don't use the full height line to separate tabs, especially see attachment 8897058 [details]. I've the feeling (but might be wrong) that this patch resulted in full height line to separate pinned tabs either, was this a wished result?
Flags: needinfo?(dao+bmo)
(In reply to Clément Lefèvre from comment #23)
> Looking at screenshots in earlier comments of the bug, it looks like
> separator between pinned tabs used to don't use the full height line to
> separate tabs, especially see attachment 8897058 [details]. I've the feeling
> (but might be wrong) that this patch resulted in full height line to
> separate pinned tabs either, was this a wished result?

This patch is only concerned with the separator between the drag space and the tab strip, not between tabs.
Flags: needinfo?(dao+bmo)
Depends on: 1403483
See Also: → 1403498
Depends on: 1403520
Blocks: 1390221
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20170928100123

This issue has been verified on latest Firefox Nightly Build ID: 20170928100123 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04 and it's not reproducible. Now, when the browser window is maximized the tab borders are correctly displayed.
Status: RESOLVED → VERIFIED
Comment on attachment 8911126 [details]
Bug 1390025 - Add separator between the tab strip and the drag space in front of it.

Approval Request Comment
[Feature/Bug causing the regression]: photon tab strip
[User impact if declined]: see attachment 8896834 [details] -- we can't ship this
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: /
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: moderately
[Why is the change risky/not risky?]: we have some followup work to do (bug 
1403483, bug 1403520) but that isn't worse than the bug fixed here
[String changes made/needed]: /
Attachment #8911126 - Flags: approval-mozilla-beta?
Comment on attachment 8911126 [details]
Bug 1390025 - Add separator between the tab strip and the drag space in front of it.

Ugly visual regression, taking it.
Should be in 57b5.
Attachment #8911126 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1404377
Depends on: 1408953
I verified this issue on Mac OS X 10.10, Windows 10 x64 and Ubuntu 16.04 with FF beta 57.0b9, and I can confirm the fix, in fullscreen the tab borders of the browser are displayed correctly.
Flags: qe-verify+
No longer depends on: 1408953
Looks like issue is back between first tab and tabstrip only if there are enough opened tabs to have the tabstrip present (with less tab, issue isn't here).

See joined screenshot.

Should this bug be reopened or a new one filed Dão?
Flags: needinfo?(dao+bmo)
(In reply to Clément Lefèvre from comment #31)
> Created attachment 8928514 [details]
> Tab bar on build 20171115100050
> 
> Looks like issue is back between first tab and tabstrip only if there are
> enough opened tabs to have the tabstrip present (with less tab, issue isn't
> here).
> 
> See joined screenshot.
> 
> Should this bug be reopened or a new one filed Dão?

Should be a new bug.
Flags: needinfo?(dao+bmo)
See Also: → 1417461
Depends on: 1440491
You need to log in before you can comment on or make changes to this bug.