New issue
Advanced search Search tips

Issue 822061 link

Starred by 14 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 11
Cc:
Components:
EstimatedDays: 0
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Feature


Sign in to add a comment

[meta] Update basic look of tabs for MD refresh

Project Member Reported by pbos@chromium.org, Mar 14 2018

Issue description

By basic look I try to mean tab shapes and colors for active / inactive / hovered tab states.
 

Comment 1 by pbos@chromium.org, Mar 14 2018

Components: UI>Browser>TabStrip

Comment 2 by bsep@chromium.org, Mar 15 2018

Cc: pkasting@chromium.org
Labels: -Pri-3 Pri-1
Owner: kylixrd@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by pbos@chromium.org, Mar 15 2018

Labels: Proj-MdRefresh
How is this different from 822063?
That's about the new tab button.  Clarified.
Blockedon: 827272
Blockedon: 827275
Labels: Target-68
… And the color picks for the incognito theme still feature extremely low contrast between the active tab and the background. (Sighs)
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 20 2018

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

commit f9f154c90e9c7b9cfaa9d72fc0bda7e36b84fc88
Author: Allen Bauer <kylixrd@chromium.org>
Date: Fri Apr 20 19:05:08 2018

Initial work for new tab design.

 o Basic shape is implemented.
 o Strokes are not drawn around and under the tabs.
 o No color and hover behaviors are changed. Next CL.
 o All changes are behind the refresh flag.

Bug:  822061 
Change-Id: I8852e4d7da3bef0fff4e6163543eee395914b2a5
Reviewed-on: https://chromium-review.googlesource.com/1020065
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552420}
[modify] https://crrev.com/f9f154c90e9c7b9cfaa9d72fc0bda7e36b84fc88/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
[modify] https://crrev.com/f9f154c90e9c7b9cfaa9d72fc0bda7e36b84fc88/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/f9f154c90e9c7b9cfaa9d72fc0bda7e36b84fc88/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/f9f154c90e9c7b9cfaa9d72fc0bda7e36b84fc88/chrome/browser/ui/views/tabs/tab_strip.h

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 23 2018

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

commit 42b63e7523c1e7098f61b9e6153139b2590efe96
Author: Allen Bauer <kylixrd@chromium.org>
Date: Mon Apr 23 23:20:15 2018

MD Refresh - Tab shape.

This CL re-enables the separator line between the toolbar and the content area.

Bug:  822061 
Change-Id: Iee0ff91b2f1b39daef89d4c9c1a09ba0142d9a08
Reviewed-on: https://chromium-review.googlesource.com/1024230
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552888}
[modify] https://crrev.com/42b63e7523c1e7098f61b9e6153139b2590efe96/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc

Comment 13 by vin...@outlook.com, Apr 26 2018

I may be wrong but I think the current separator line between the toolbar and the content area is a little bit darker than it looks in the Comment 9 and that makes it looks very out of place comparing to light theme on the toolbar.
Project Member

Comment 14 by bugdroid1@chromium.org, Apr 26 2018

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

commit 628df6f75300f96d19eb04e87aee176e0f19c8d4
Author: Peter Kasting <pkasting@chromium.org>
Date: Thu Apr 26 18:14:19 2018

Update tab height for MD refresh.

BUG= 822061 
TEST=none

Change-Id: I1b301e28c7c374f3605e74ddb8610a5c82b133a1
Reviewed-on: https://chromium-review.googlesource.com/1029173
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554083}
[modify] https://crrev.com/628df6f75300f96d19eb04e87aee176e0f19c8d4/chrome/browser/ui/layout_constants.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 30 2018

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

commit e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a
Author: Allen Bauer <kylixrd@chromium.org>
Date: Mon Apr 30 19:31:08 2018

Material Refresh - Inactive tab colors and behavior.

 * Inactive tabs
  * No shape or color separate from the background.
  * Shape and color appears on hover.
  * No visible close icon until mouse hover.
  * Incognito mode colors for close icon for static/hover/pressed.

TBR=msw@chromium.org

Bug:  822061 
Change-Id: I0898fcb49f1133d0922f364c0453b4436e344b35
Reviewed-on: https://chromium-review.googlesource.com/1024602
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554846}
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/app/vector_icons/BUILD.gn
[add] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/app/vector_icons/tab_close_button_highlight.icon
[add] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/app/vector_icons/tab_close_button_touch_highlight.icon
[delete] https://crrev.com/d2dc7a9eacc4ffa5832e769f1b5a3ddb10f808db/chrome/app/vector_icons/tab_close_button_touch_hovered_pressed.icon
[delete] https://crrev.com/d2dc7a9eacc4ffa5832e769f1b5a3ddb10f808db/chrome/app/vector_icons/tab_close_button_touch_incognito_hovered_pressed.icon
[delete] https://crrev.com/d2dc7a9eacc4ffa5832e769f1b5a3ddb10f808db/chrome/app/vector_icons/tab_close_hovered_pressed.icon
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/browser/themes/theme_properties.cc
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/browser/themes/theme_service.cc
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/browser/ui/cocoa/hover_close_button.mm
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/browser/ui/views/tabs/tab_close_button.cc
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/browser/ui/views/tabs/tab_close_button.h
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/browser/ui/views/tabs/tab_controller.h
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/browser/ui/views/tabs/tab_strip.h
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/e35ec3e14e0dab0496ebfe1c9b63ff5973e33c0a/ui/gfx/color_palette.h

1. The close-x button needs to be visible on hover of inactive tabs for as long as possible. There should be a threshold for when we decide to hide the close-x button on hover. I'm thinking that threshold is 52px (see attachment). A tab that's less than the 52px threshold only shows the favicon or a media indicator when applicable. 

2. The space between left-side of tab and left-side of favicon should be 12dp. Right now it's 8dp. Matching the same space on the RHS of the tab is not intended. 

3. Hide the divider for the first tab only. We could use RHS dividers instead, but we'd have to hide the divider on focused tabs so I think the designs are equal. 

4. Update divider height to 20dp. A longer divider is going to break up the monotony in a cluttered tab strip, but will also vertically center itself amongst a 36px tab.

5. Update the divider color to be 25% of the expected ntb color (Google Grey 700 / #5F6368). 


* All this applies to Touchable Chrome as well 
** The color specs here can apply to the divider between the secure chip and URL, but the height should remain 16dp. 
01_actual.png
49.1 KB View Download
02_height=20dp.png
49.1 KB View Download
03_25%.png
116 KB View Download
03_today-with-20.png
116 KB View Download
Forgot to attach an illustration of point #1 in c17. IMO, the order in which things should hide for inactive tabs goes as follows:

- Page title
- Close-x
- Favicon
- Media indicator

close-x.png
42.5 KB View Download
For (1), it's not clear to me what you're saying.  Is this correct?:

  * Chrome has always hidden close buttons on inactive tabs once those tabs get too small
  * In Refresh, we also hide close buttons on inactive tabs at any size when those tabs are not hovered, so the above bullet becomes applicable only to hovered inactive tabs
  * But, for Refresh we should lower the size threshold for that first bullet to 52 DIP

If so, we should already be fairly close to 52 DIP (between the separators), but I'm not sure of the exact value because we don't have any of the within-tab padding/layout done yet.  I would say "wait and retest when that's in.

For (2), again, this hasn't even been looked at yet -- not ready for review.  That said, as tabs shrink, we currently shrink the space between the left side of the tab and the favicon, first at the time when we hide close buttons, and then continually once we only have room for the favicon.  We definitely still need to do the latter; less clear on the former.  Subject to further testing once we get there.

For (3), I don't know what you mean by "first tab only".  Are you talking about the drag-multiselection case?

(4) and (5) are doable now, though with an asterisk on (5); the new tab button color is itself not being computed correctly yet, since we need to compute it in a way that works for themes, and thus it should probably match the caption icons.  Also, 25% on a 1 DIP line is going to be invisible to large fractions of our users -- I can't imagine this meets minimum contrast specs.
OK, the illustration cleared up what you were saying on #1.  Yeah, just wait on this until we get all the tab layout done, it hasn't been started yet.
Cc: kylixrd@chromium.org
 Issue 841630  has been merged into this issue.
>> first at the time when we hide close buttons, and then continually once we only have room for the favicon.  We definitely still need to do the latter; less clear on the former.  Subject to further testing once we get there.

I'm not convinced anything needs to change there either. Agreed to wait till we get there. 

>> For (3), I don't know what you mean by "first tab only".  Are you talking about the drag-multiselection case?

Nope, just talking about the first tab on the strip, right of the ntb on windows, right of the window controls on mac. 

>> (4) and (5) are doable now, though with an asterisk on (5); the new tab button color is itself not being computed correctly yet, since we need to compute it in a way that works for themes, and thus it should probably match the caption icons.  

Right. I'm assuming that the computation for a dark color is going to be something close to Google Grey 700 so 25% of whatever that computation is what I'm aiming for. 

>> Also, 25% on a 1 DIP line is going to be invisible to large fractions of our users -- I can't imagine this meets minimum contrast specs.

It likely doesn't but I want to start aggressive and throttle up to more conservative values. As someone who's lived without dividers for ~3 weeks, my gut tells me that the dividers aren't adding a lot of value. In other words, using Chrome without dividers wasn't all that terrible. 

The original spec had dividers at GG900 35% which is lighter than what's built (looks more like #000 35%), but darker than what I'm requesting. So there's plenty of conservative options to consider, but if we could start with 25%, I think it'd be a good starting point for the pizza team. 
Wait, so for (3) you want there to be no divider between NTB and tabs?  That seems... confusing.

For the dividers, Allen and I both thought they made things look more readable and less busted when we added them... I'm kinda skeptical of "they don't add value" because if they really don't I'd push for "don't ship them".  If we ship something that only some of our customers see, that seems worse than either extreme.
>> Wait, so for (3) you want there to be no divider between NTB and tabs?  That seems... confusing.

That's correct. It's a divergence from the spec, but a good one IMO. 

Re: dividers
The dividers add value, but what I'm saying is that they aren't solely responsible for distinguishing tabs. To my surprise, they're sharing that responsibility with the hover highlight and the general tab layout provided. All together, those three things help your brain distinguish tab boundaries, meaning that the dividers don't have to slap you in the face with a 4.5 ratio. 

---

Respectfully, let's please not deliberate on this any further. The sooner we get the fix, the sooner we can share with the pizza team. We can pick this conversation back up when we have more eyes on it. 
For (4), the current positioning of the divider is wrong. It's not being vertically aligned properly. I have a fix for that, so maybe wait for that before we change its height.

Right now it matches the height of the close button icon of 16dip. For touch mode, it goes to 24dip, shouldn't the separator then also go to 24dip or should it remain 16?
> The original spec had dividers at GG900 35% which is lighter than what's built (looks more like #000 35%), but darker than what I'm requesting. So there's plenty of conservative options to consider, but if we could start with 25%, I think it'd be a good starting point for the pizza team.

Correct. The current color is calculated base on 35% of #000. This was the point of what we discussed about changing BlendTowardOppositeLuma to use GG900 instead of #000 for the dark baseline. Once that is done, the divider will then be GG800 35%. Would it be valuable to wait until then to decide?

Comment 27 by pbos@chromium.org, May 10 2018

I think the current divider color based on black is jarringly strong and moving from 000 to GG900 base color won't fix that. I wouldn't hold up getting a lighter shade in there on doing the luma change so we can get it into early testing, but those are my 2c.
My concern would be that once the luma change goes in, it would end up being too light. I also don't want to be churning on this.

As for whether separators should be there or not, I think we should wait until the next CL lands. The close button and the separators will now animate in and out in conjunction with the tab hover highlight. The appearance/disappearance of them are much less visually jarring. 
Project Member

Comment 29 by bugdroid1@chromium.org, May 10 2018

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

commit bac1ab5cd4c541a867b9a56adaef1ba2b8d83629
Author: Allen Bauer <kylixrd@chromium.org>
Date: Thu May 10 18:42:36 2018

Material Refresh - Now fades in/out the tab separator and the close button in conjunction with the glow hover effect.

Bug:  822061 
Change-Id: Ic4c339c54a61d7f00ff8eedc1ae20c6176440aec
Reviewed-on: https://chromium-review.googlesource.com/1052382
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557601}
[modify] https://crrev.com/bac1ab5cd4c541a867b9a56adaef1ba2b8d83629/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/bac1ab5cd4c541a867b9a56adaef1ba2b8d83629/chrome/browser/ui/views/tabs/tab_close_button.cc
[modify] https://crrev.com/bac1ab5cd4c541a867b9a56adaef1ba2b8d83629/chrome/browser/ui/views/tabs/tab_close_button.h

tl;dr from email thread: 
- please make the change to 20dp divider heights
- GG800 35% is a satisfactory color to start with
- Sharing with the pizza team now is not imperative 

bettes@ - What should the divider height be for touch? The tab is taller under touch.
I think the page titles increase from 12 to 14 (hopefully not 15) so let's bump the dividers up to 24dp. Thanks!
Can we hide dividers for pinned tabs as well? 
So hide divider if 1.) it's the first tab in the strip and/or 2.) it's a pinned tab. 

With a cluttered tab strip, it gives off a nice distinguishing factor. 
divide.png
35.6 KB View Download
Blockedon: 842798
Project Member

Comment 35 by bugdroid1@chromium.org, May 15 2018

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

commit 035552498218737a9741615c1751136edf9cd6ad
Author: Allen Bauer <kylixrd@chromium.org>
Date: Tue May 15 15:47:38 2018

Material Refresh - Make separator 20dip instead of 16dip. Always show close button on active tabs.

GetOpacity() didn't handle the active tab case where the close button should be always visible.



Bug:  822061 
Bug:  842218 
Change-Id: Id91e5ab402fb1e9e249f8cb60d1a26a3c9a3c46e
Reviewed-on: https://chromium-review.googlesource.com/1055648
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558717}
[modify] https://crrev.com/035552498218737a9741615c1751136edf9cd6ad/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/035552498218737a9741615c1751136edf9cd6ad/chrome/browser/ui/views/tabs/tab_close_button.cc

EstimatedDays: 3
EstimatedDays: 4
EstimatedDays: 5
Blockedon: 847688
EstimatedDays: 2
Labels: -Type-Bug Type-Feature
Hello pkasting@: I noticed that your change from #c14 has caused a "dead-drag" area at the bottom of the Tabstrip. Please see the red marked area in the enclosed screenshot. Is this already a known issue? If not, then please let me know, if I should file a separate report for it. Thanks :)
dead-drag-space.png
79.8 KB View Download
That sounds like some MacViews-specific issue.  I would file it.
Thanks for your feedback. I will file it.
EstimatedDays: 1
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Allen, what remains to be done on this bug that isn't covered by other bugs?

* 24 DIP dividers for touch?
* Is the divider color fully covered by  bug 841271 ?
* Anything else?

If it's just the touch divider height this is 1 day.
Blockedon: 848412
> * 24 DIP dividers for touch?

Done.

> * Is the divider color fully covered by  bug 841271 ?

Yes, it should be.

> * Anything else?

Unless we're including the drag area in this, then I can't think of anything else beyond your current work for tab layout.
Blockedon: 848415
Blockedon: 848416
EstimatedDays: 0
Summary: [meta] Update basic look of tabs for MD refresh (was: Update basic look of tabs for MD refresh)
Drag area is part of the frame, so let's change this to 0 days and file anything remaining as blockers.  This is now a pure metabug.
Blockedon: 848474
Blockedon: 848482
Blockedon: 848546
Project Member

Comment 53 by bugdroid1@chromium.org, Jun 4 2018

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

commit ba72b0c2fe924d8d71444dc63985e087dabc425e
Author: Allen Bauer <kylixrd@chromium.org>
Date: Mon Jun 04 20:55:20 2018

Adjust left side padding before favicon or tab title to refresh spec.

The material refresh spec places the favicon or title text 12dips from the perceived left edge of the tab, which is the vertical edge centered in the end cap.

Bug:  822061 
Change-Id: Ibee363fb0849ae7216e854f354db77cec7bd50fb
Reviewed-on: https://chromium-review.googlesource.com/1076779
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564229}
[modify] https://crrev.com/ba72b0c2fe924d8d71444dc63985e087dabc425e/chrome/browser/ui/layout_constants.cc
[modify] https://crrev.com/ba72b0c2fe924d8d71444dc63985e087dabc425e/chrome/browser/ui/views/tabs/tab.cc

Owner: markchang@chromium.org
Moving meta bugs to PM to make eng bandwidth more clear.
Project Member

Comment 55 by bugdroid1@chromium.org, Jun 14 2018

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

commit 77265c6e7b0219a673cf3c4ad9316b732c705d8b
Author: Bret Sepulveda <bsep@chromium.org>
Date: Thu Jun 14 02:31:54 2018

Fix Refresh tab icon layout when the tab is small.

Refresh changes the amount of extra padding added to the left of the
favicon. But when the tab is too small to accommodate this extra
padding, Refresh was failing to remove this padding and calculate
icon visibility correctly.

This also fixes TabTest.LayoutAndVisibilityOfElements when Refresh is
enabled.

Bug:  846410 ,  822061 
Change-Id: I2b77b976d55e314da3e6b5193403eb2a102dd426
Reviewed-on: https://chromium-review.googlesource.com/1098148
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567098}
[modify] https://crrev.com/77265c6e7b0219a673cf3c4ad9316b732c705d8b/chrome/browser/ui/views/tabs/tab.cc

Labels: Meta
Blockedon: 854371
Blockedon: 854722
Blockedon: 854738
Blockedon: 855091
Blockedon: 855729
Blockedon: 856342
Blockedon: 856345
Blockedon: 856492
Blockedon: 856896

Comment 66 by stan...@gmail.com, Jun 28 2018

With the refresh, the tab height was increased quite a bit, I am sure many people will have a problem with that, losing space and all. Any chance this could be reconsidered? It also does look a bit weird currently, top and bottom padding of the tab title text are way more than left and right.
There are ongoing considerations around that question.  Getting sufficient height to provide room for a drag handle is a complicating factor.
comment 66 and 67, perhaps a Compact/Default/Touch density menu is the best solution. Firefox has that, as does Gmail
Blockedon: 859327
Blockedon: 859620
Blockedon: 856893
Labels: -Proj-MdRefresh Proj-DesktopUI
Labels: Hotlist-DesktopUITriaged
Labels: Group-Feature_Process
Status: Fixed (was: Assigned)

Sign in to add a comment