Remove final tab separator when the tab shapes are visible |
||||||||||||||||||||
Issue descriptionWhen 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.
,
Jul 11
,
Jul 11
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.
,
Jul 11
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.
,
Jul 11
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.
,
Jul 11
With the NTB trailing the last tab, I prefer that we keep the final tab separator. This is WontFix?
,
Jul 11
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.
,
Jul 12
,
Jul 12
,
Jul 12
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".
,
Jul 17
,
Aug 14
,
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
,
Aug 20
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.
,
Aug 20
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
,
Aug 20
Is the change listed at #13 looking good in canary and safe to merge now?
,
Aug 20
,
Aug 20
,
Aug 21
@16: Verified working in canary. Can be merged during the process of merging the changes on bug 862276 .
,
Aug 21
Thank you pkasting@. Approving merge to M69 branch 3497 based on comments #14 and #19.
,
Aug 21
,
Aug 21
,
Aug 21
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
,
Aug 21
Fixed on trunk and M69
,
Aug 22
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.
,
Aug 22
I'm investigating. Other changes have landed on branch depending on this so reverting is not ideal. Hoping the fix is trivial.
,
Aug 22
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().
,
Aug 22
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.
,
Aug 22
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.
,
Aug 23
I'm going to leave this fixed and let bug 875707 track the other issue, since we don't look to be reverting.
,
Sep 13
|
||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||
Comment 1 by bsep@chromium.org
, Jul 10