[meta] Update basic look of tabs for MD refresh |
||||||||||||||||||||||||||||||||||||||
Issue description
,
Mar 15 2018
,
Mar 15 2018
,
Mar 16 2018
How is this different from 822063?
,
Mar 16 2018
That's about the new tab button. Clarified.
,
Mar 29 2018
,
Mar 29 2018
,
Apr 3 2018
,
Apr 19 2018
… And the color picks for the incognito theme still feature extremely low contrast between the active tab and the background. (Sighs)
,
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
,
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
,
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.
,
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
,
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
,
May 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1c682e4932d0f424d06c751d3351dec8e639b5e6 commit 1c682e4932d0f424d06c751d3351dec8e639b5e6 Author: Allen Bauer <kylixrd@chromium.org> Date: Tue May 08 16:14:25 2018 Material Refresh - Paint the separator on the left side of inactive tabs. TBR=asvitkine@chromium.org Bug: 822061 Change-Id: I5098cf8cd40bd691636ceed52fb27d6709fae5fc Reviewed-on: https://chromium-review.googlesource.com/1040661 Commit-Queue: Allen Bauer <kylixrd@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#556818} [modify] https://crrev.com/1c682e4932d0f424d06c751d3351dec8e639b5e6/chrome/browser/ui/views/tabs/tab.cc [modify] https://crrev.com/1c682e4932d0f424d06c751d3351dec8e639b5e6/chrome/browser/ui/views/tabs/tab.h [modify] https://crrev.com/1c682e4932d0f424d06c751d3351dec8e639b5e6/chrome/browser/ui/views/tabs/tab_controller.h [modify] https://crrev.com/1c682e4932d0f424d06c751d3351dec8e639b5e6/chrome/browser/ui/views/tabs/tab_strip.cc [modify] https://crrev.com/1c682e4932d0f424d06c751d3351dec8e639b5e6/chrome/browser/ui/views/tabs/tab_strip.h [modify] https://crrev.com/1c682e4932d0f424d06c751d3351dec8e639b5e6/chrome/browser/ui/views/tabs/tab_unittest.cc [modify] https://crrev.com/1c682e4932d0f424d06c751d3351dec8e639b5e6/ui/gfx/color_palette.h
,
May 10 2018
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.
,
May 10 2018
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
,
May 10 2018
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.
,
May 10 2018
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.
,
May 10 2018
,
May 10 2018
>> 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.
,
May 10 2018
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.
,
May 10 2018
>> 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.
,
May 10 2018
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?
,
May 10 2018
> 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?
,
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.
,
May 10 2018
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.
,
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
,
May 10 2018
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
,
May 11 2018
bettes@ - What should the divider height be for touch? The tab is taller under touch.
,
May 14 2018
I think the page titles increase from 12 to 14 (hopefully not 15) so let's bump the dividers up to 24dp. Thanks!
,
May 14 2018
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.
,
May 14 2018
,
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
,
May 29 2018
,
May 29 2018
,
May 29 2018
,
May 30 2018
,
May 31 2018
,
May 31 2018
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 :)
,
May 31 2018
That sounds like some MacViews-specific issue. I would file it.
,
May 31 2018
Thanks for your feedback. I will file it.
,
May 31 2018
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.
,
May 31 2018
,
May 31 2018
> * 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.
,
May 31 2018
,
May 31 2018
,
May 31 2018
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.
,
May 31 2018
,
May 31 2018
,
Jun 1 2018
,
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
,
Jun 11 2018
Moving meta bugs to PM to make eng bandwidth more clear.
,
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
,
Jun 14 2018
,
Jun 19 2018
,
Jun 20 2018
,
Jun 20 2018
,
Jun 21 2018
,
Jun 22 2018
,
Jun 25 2018
,
Jun 25 2018
,
Jun 26 2018
,
Jun 27 2018
,
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.
,
Jun 28 2018
There are ongoing considerations around that question. Getting sufficient height to provide room for a drag handle is a complicating factor.
,
Jun 28 2018
comment 66 and 67, perhaps a Compact/Default/Touch density menu is the best solution. Firefox has that, as does Gmail
,
Jul 1
,
Jul 2
,
Jul 4
,
Sep 20
,
Sep 26
,
Oct 8
,
Oct 11
|
||||||||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||||||||||||||||
Comment 1 by pbos@chromium.org
, Mar 14 2018