New issue
Advanced search Search tips

Issue 862438 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 23
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Blocking:
issue 820495



Sign in to add a comment

Remove final tab separator when the tab shapes are visible

Project Member Reported by bsep@chromium.org, Jul 10

Issue description

When tab shapes are visible in Refresh (themes and Windows 7) the rightmost tab has a separator, which looks pretty odd. Screenshot attached. We may want to just remove separators entirely in those cases.
 
theme-right-separator.PNG
1.6 MB View Download
Owner: pbos@chromium.org
Components: UI>Browser>TabStrip
For Win 7 glass, it's probably safe to say that we can remove separators at both edges, since we know that's a pretty clear visual contrast.  (P.S. I still haven't seen a screenshot of this.)

I don't know that we can assume themes make tab shapes visible, however.  (We could decide themes should draw strokes instead, but that's a different thing.)  In your screenshot, for example, the tab shape is pretty subtle, and I would perhaps prefer the separator to stay than go.
Cc: pkasting@chromium.org bettes@chromium.org
Maybe this horse has already seen better days, but do we ever need a trailing separator? I don't think anyone should be trying to hit the RHS tab after the close-x which is now always visible?

I don't really think removing the trailing divider looks weird even without a tab shape.
divider_close_x.png
25.6 KB View Download
When refering to the "RHS tab after close-x" I mean the part of the tab marked purple. I don't think users should be trying to hit it in the first place, so divider to mark this area's end feels less important.
post-close-x-hittarget.png
5.1 KB View Download
With the NTB trailing the last tab, I prefer that we keep the final tab separator. This is WontFix? 
When there aren't visible tab shapes (e.g. single tab mode, or background tabs in the default theme), having a separator makes things look less glitchy, so we shouldn't remove it.

But I think we should remove it when the tab shape is clearly visible (Win 7, maybe some themes), for the same reason we don't put separators on active tabs; it looks buggy there.
Labels: Group-Tabstrip
Labels: -Group-Tabstrip -Pri-2 Group-Themes Pri-3
Labels: -Pri-3 OS-Windows Pri-2
Elevating to P2; this is confirmed problematic on Win 7, and the guidance from Mark (which I agree with) is that Win 7 users matter a lot more than theme users.

I suspect that we want to key this off "is the glass frame with custom titlebar disabled".
Cc: pbos@chromium.org
Owner: pkasting@chromium.org
Labels: M-69
Status: Started (was: Assigned)
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e220a7a7a89ca0ae79114c65aa2904a75901b28

commit 1e220a7a7a89ca0ae79114c65aa2904a75901b28
Author: Peter Kasting <pkasting@chromium.org>
Date: Sat Aug 18 00:39:33 2018

Hide separators on ends when tab shapes are visible.

Bug:  862438 
Change-Id: I51e0b61d14184e0f94467bc957240091d77897f3
Reviewed-on: https://chromium-review.googlesource.com/1173755
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584263}
[modify] https://crrev.com/1e220a7a7a89ca0ae79114c65aa2904a75901b28/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
[modify] https://crrev.com/1e220a7a7a89ca0ae79114c65aa2904a75901b28/chrome/browser/ui/views/tabs/browser_tab_strip_controller.h
[modify] https://crrev.com/1e220a7a7a89ca0ae79114c65aa2904a75901b28/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc
[modify] https://crrev.com/1e220a7a7a89ca0ae79114c65aa2904a75901b28/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h
[modify] https://crrev.com/1e220a7a7a89ca0ae79114c65aa2904a75901b28/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/1e220a7a7a89ca0ae79114c65aa2904a75901b28/chrome/browser/ui/views/tabs/tab_controller.h
[modify] https://crrev.com/1e220a7a7a89ca0ae79114c65aa2904a75901b28/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/1e220a7a7a89ca0ae79114c65aa2904a75901b28/chrome/browser/ui/views/tabs/tab_strip.h
[modify] https://crrev.com/1e220a7a7a89ca0ae79114c65aa2904a75901b28/chrome/browser/ui/views/tabs/tab_strip_controller.h
[modify] https://crrev.com/1e220a7a7a89ca0ae79114c65aa2904a75901b28/chrome/browser/ui/views/tabs/tab_unittest.cc

Labels: Merge-Request-69
Requesting merge; rationale is that this is pretty high-visibility (looks glitchy in all cases for all Win 7 users) and may be necessary for cleanly landing the patches in  bug 862276 , since it's part of a chain of fixes landed there.
Project Member

Comment 15 by sheriffbot@chromium.org, Aug 20

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Is the change listed at #13 looking good in canary and safe to merge now?
Labels: -M-69 M-X
Labels: -M-X M-69
@16: Verified working in canary.  Can be merged during the process of merging the changes on  bug 862276 .
Labels: -Merge-Review-69 Merge-Approved-69
Thank you pkasting@.
Approving merge to M69 branch 3497 based on comments #14 and #19. 
Labels: Proj-DesktopUI
Labels: Target-69
Project Member

Comment 23 by bugdroid1@chromium.org, Aug 21

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/46e816f41ab538af9dbfe487610463cc460acd8a

commit 46e816f41ab538af9dbfe487610463cc460acd8a
Author: Peter Kasting <pkasting@chromium.org>
Date: Tue Aug 21 23:36:21 2018

Hide separators on ends when tab shapes are visible.

Bug:  862438 
Change-Id: I51e0b61d14184e0f94467bc957240091d77897f3
Reviewed-on: https://chromium-review.googlesource.com/1173755
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#584263}(cherry picked from commit 1e220a7a7a89ca0ae79114c65aa2904a75901b28)
Reviewed-on: https://chromium-review.googlesource.com/1184281
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#755}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/46e816f41ab538af9dbfe487610463cc460acd8a/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
[modify] https://crrev.com/46e816f41ab538af9dbfe487610463cc460acd8a/chrome/browser/ui/views/tabs/browser_tab_strip_controller.h
[modify] https://crrev.com/46e816f41ab538af9dbfe487610463cc460acd8a/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc
[modify] https://crrev.com/46e816f41ab538af9dbfe487610463cc460acd8a/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h
[modify] https://crrev.com/46e816f41ab538af9dbfe487610463cc460acd8a/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/46e816f41ab538af9dbfe487610463cc460acd8a/chrome/browser/ui/views/tabs/tab_controller.h
[modify] https://crrev.com/46e816f41ab538af9dbfe487610463cc460acd8a/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/46e816f41ab538af9dbfe487610463cc460acd8a/chrome/browser/ui/views/tabs/tab_strip.h
[modify] https://crrev.com/46e816f41ab538af9dbfe487610463cc460acd8a/chrome/browser/ui/views/tabs/tab_strip_controller.h
[modify] https://crrev.com/46e816f41ab538af9dbfe487610463cc460acd8a/chrome/browser/ui/views/tabs/tab_unittest.cc

Status: Fixed (was: Started)
Fixed on trunk and M69
Status: Assigned (was: Fixed)
The fix for this caused  bug 875707 . I am reverting it in https://chromium-review.googlesource.com/c/chromium/src/+/1185103 .

This is at least DCHECKing in debug builds. It possibly may crash in release mode; I don't know. It will likely need to be removed from the m69 branch.
I'm investigating.  Other changes have landed on branch depending on this so reverting is not ideal.  Hoping the fix is trivial.
Pausing the CQ on the revert by request of Peter.

Repro for this DCHECK:
1. Go to youtube.com
2. Click a video
3. Click the fullscreen button

[52799:1295:0822/125842.969688:FATAL:browser_non_client_frame_view.cc(110)] Check failed: browser_view_->IsTabStripVisible(). 

Re #26, We're planning to cut M69 Beta RC for release tomorrow today @ 1:00 PM PT. Pls keep the bug updated on progress so I can plan accordingly for Beta RC cut. Thank you. 
What we've determined so far as that this should have no consequence for M69; it's a failed DCHECK with no bad effects.  Once we have a fix on trunk we may try to merge to M69 anyway so that debug builds of M69 don't have problems; it looks like that fix will be connected to not painting the tabstrip on Mac when it's not visible.
Status: Fixed (was: Assigned)
I'm going to leave this fixed and let  bug 875707  track the other issue, since we don't look to be reverting.
Labels: Hotlist-MdRefreshDesignPolish

Sign in to add a comment