Extend TabbedPane view to support side tabs layout and different tab label styles. |
||||||||||||||||||||||||||||||||||
Issue descriptionThis new UI comes from a recent design [1], which requires a side tabbed pane view layout and the tab is a half rounded corner label/button view. [1] https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZaz7gfsqSw6O/files/MCFLw-dbpBT3cOf7zFQbjSXZwMSJbxAVNpw Looks like we can extend current TabbedPane view to support this, we need to add: 1. Side tab layout. 2. The tab can set its own TabLabel style for customized shaped/colored label/button view.
,
Dec 11 2017
,
Dec 11 2017
That design looks like WebUI, not Views - and the existing settings page is WebUI. Are we planning to redo it in Views? Anyway, this specific bug seems doable. I'm not quite sure how to abstract the tab style nicely - perhaps allow the client class to supply its own Tab class somehow?
,
Dec 11 2017
ellyjones@, we plan to implement this Keyboard Shortcut Viewer (KSV) in native Views. For requirement 1: we can add a flag to indicate horizontal/vertical layout. For requirement 2: I can implement a specific subclass of the Tab for the KSV view. Is it ok to VIEWS_EXPORT the Tab as well?
,
Dec 12 2017
Adding a horizontal/vertical layout flag sounds good to me. I think it's fine to VIEWS_EXPORT the Tab class, but you will also need to customize the drawing of the tabstrip, so you'll need to VIEWS_EXPORT TabStrip as well. Feel free to send me code reviews for this code - I'm most familiar with TabbedPane, but I'm not a Views owner so you'll also need one of those.
,
Dec 27 2017
There is a new mock up for the UI: https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZaz7gfsqSw6O/files/MCHAPli2-HuKj1UuVjdU8pJ9wMSJbxAVNpw For the initial cl, I plan to put KSVTabbedPane, KSVTabStrip, and KSVTab under components/KSV. If we think we can reuse and generalize them, we can move or merge them to ui/views/controls/tabbed_panes. From the new mock up of the KSV UI, the KSVTabbedPane, KSVTabStrip, and KSVTab can set the followings: For Tab: 1. Can set the rounded radius for the right side two corners. 2. Can define the background color. 3. Can define the preferred size. 4. Can define the insets/border for its label. 5. Can set its label's own style, such as color, font, alignment. 6. Can set the highlighted color for label and background: need to override OnStateChanged() and access enum TabState. For TabStrip: 1. Set the BoxLayout layout direction and child spacing. 2. Not need to paint tab_strip border, or the border just a straight line (need to be finalized by UX). For the TabbedPane: 1. Can layout TabStrip and contents side by side. 2. Can set its TabStrip insets. 3. Can set its content insets. 4. Can set its preferred size.
,
Dec 28 2017
Seems we can make those in #6 more generic by adding ctors and functions for: 1. Tab can take a Label* as the init parameter, so that we can style the text in the label. 2. Tab can set the font colors and background colors for active/inactive/hoverred states. 3. TabStrip can take a LayoutManager* as the init parameter, so that we can horizontal/vertical align the tabs. 4. Override TabStrip OnPaintBorder for vertical aligned tabs. 5. TabbedPane can take a TabStrip* as the init parameter. 6. TabbedPane can add Tab*, so that we can provide customer tabs with rounded corners and background. 7. TabbedPane can set the Insets for tab_strip and contents, so that we can have customer alignments shown in the mock up.
,
Jan 2 2018
I think the right way to expose the capabilities mentioned in #6 is via a delegate or drawing provider, like <https://cs.chromium.org/chromium/src/ui/views/controls/tree/tree_view_drawing_provider.h?q=TreeViewDrawingProvider&sq=package:chromium&l=23> - I prefer that to having subclasses override methods from TabbedPane and its helper classes. Something like: class TabbedPaneDrawingProvider { SkScalar GetTabCornerRadius() SkColor GetTabBackgroundColor() gfx::Size GetTabPreferredSize() ... and so on ... } does that seem reasonable? For the suggestions in #7, I'd rather that we not expose the inner workings of TabbedPane like this unless there's no other option; I'd rather use the drawing provider / delegate approach if it can work.
,
Jan 2 2018
I'm not so sure a delegate should provide that information. In the case of the CookiesTreeViewDrawingProvider, it's customizing content within the control (ie. showing 'annotations' inside the TreeView with different opacity, color, icon, etc.). That's somewhat reasonable, but even there I'd rather see a way to set properties on the TreeView content via data structures rather than instantiating a custom drawing delegate with callbacks for each node. In this scenario, the tabbed pane control is taking on a second standard appearance/mode. Ideally, there would be one flag for end-users to switch: TabbedPane::SetUseHorizontalLayout(bool use_horizontal_layout). The KSV UI shouldn't need to specify appearance details for the tabs, that should be managed by the TabbedPane class and its helper classes, just like in the current tabbed pane mode. (ie. there isn't currently a TabbedPane::SetTabCornerRadius(), and this new mode shouldn't require that either). As for the internal implementation, I'd suggest something simple like giving the internal Tab class a similar bool for horizontal/vertical (side/top) layout, or checking TabbedPane::use_horizontal_layout(), not passing around a separate delegate. I hope that viewpoint is helpful. Also, can we remove Restrict-View-Google here? I'm not sure why that's needed.
,
Jan 2 2018
@#9, we can make the tabbed_pane taking on a second standard appearance/mode. There is not too much customization in tabbed_pane (we need the helper to define the relative position of content to tab_strip). One flag to indicate horizontal/vertical would work. However, we still need either subclasses or some drawing provider of Tab/TabStrip for customized font, color, insets, layout, borders of the tab and tab_strip, as mentioned in #6. KSV UI need to set those parameters. p.s. Since the launcher bug 755448 of the KSV UI is marked by Restrict-View-Google, so I add the same restrict here. ovanieva@, is it ok to remove the restriction?
,
Jan 2 2018
Why does KSV need to set those appearance properties? Shouldn't that just be the default appearance for a horizontal TabbedPane control? The cookies dialog doesn't set the font for its tabs, so neither should KSV. Here's the rough pattern I'm suggesting:
void TabbedPane::SetUseHorizontalLayout(bool use_horizontal_layout) { // Similar for Tab, TabStrip, etc.
if (use_horizontal_layout) {
// Set the font, color, insets, layout, borders of the tab and tab_strip (horizontal)
} else {
// Set the font, color, insets, layout, borders of the tab and tab_strip (vertical)
}
}
My first inclination is to just modify the existing TabbedPane code with a bool flag for the horizontal/vertical mode; you shouldn't need subclasses/delegates if you just have the existing elements check that flag. That said, as with your StyledLabel-like work, if you need to build a separate control, that's probably fine, but the interface and internal workings should parallel TabbedPane pretty closely. (so it's easier to merge the two impls later on, or replace either impl with another that supports both modes).
,
Jan 11 2018
> we plan to implement this Keyboard Shortcut Viewer (KSV) in native Views. Is there a mailing-list/doc/somewhere else this was discussed? I am curious to learn what considerations lead to this decision to do it in views, instead of doing it in web technologies.
,
Jan 12 2018
sadrul@, I think you find the design doc of this. Thanks.
,
Jan 12 2018
Thank you msw@ and ellyjones@ reviewing the prototype cl. elizabethchiu@ and shibasheikh@, some comments for UX are that: It would be best for the new side tab UI specs to be derived from LayoutProvider, NativeTheme, layout_constants, etc., rather than defining local magic numbers that aren't consistent with the rest of the UI. For examples 1. The corner radius of the tab, 2. The highlighted colors. Try to follow how the existing pattern with kTabTitleColor_Active, etc. Or better yet, integrate NativeTheme use. It would be great if this control could properly handle high contrast mode coloring (presuming that's still a thing on Chrome OS).
,
Jan 12 2018
Hi ellyjones@ and msw@, To move forward, would it make sense I put out a cl to support the side tab without new UX specs yet? The styling will be consistent with current horizontal tabs styling for the first cl. When the Chrome OS UX specs are ready and then we put another cl only for the styling changes?
,
Jan 12 2018
That sounds good to me. The initial TabbedPane changes seem fairly minimal and straightforward. That said, I'm interested if Sadrul thinks we should pursue WebUI here (embedding some WebView in Ash, like AppList answer cards). I hope we'll have a consensus before landing Views changes.
,
Jan 12 2018
,
Jan 15 2018
[My response got a little out of hand, sorry about that] The notable things in the mock I see: a big scrollable view, containing rows of potentially multi-line text with mixed styles, with sticky section headers. This is obviously possible to do in views, but I think it will be fairly difficult: . The network-list in ash system-tray popup has some sticky section-headers. It was a fair amount of work to get that done, with some tricky edge cases to deal with. I am not sure how much of that work applies here for KSV, or can be used for KSV, but I will be pleasantly surprised (and happy!) if this can be done easily. . The scroll in the scroll-view is not synchronized with the compositor. So if you are scrolling, you will see stuttering/jank. The network-list (or other such native lists, e.g. bluetooth list etc.) are small enough that such jank is not very noticeable right now. For the big list in KSV, I think the jank will be fairly noticeable. Fixing this will need some work (although this shouldn't be too difficult I think). . I think updating styles of parts of some text (background, border, etc. for the keys, bolding the parts of the text that match the entered search) will be tricky, especially as you enter keys in the search box and update the styles, you will likely need to re-layout etc. too. I guess what I am saying is, I think that doing this correctly in views will not be easy. From the work I have seen happen for material-design, my take is that it needed very specific fixes for individual cases, and there were not a lot of general changes that we were able to apply everywhere. I suspect this will be the case for material-design v2 too. So while we can experiment with MDv2 for native-views in KSV, I don't think a lot of that work will be useful for other parts of ash or chrome UI. On the other hand, I think doing the KSV UI in web will be easier in comparison. The compositor is optimized for scrolling web-pages, so it will be easier to get good performance. Iterating over the various versions of the mocks should also be easier. PM/UX/developers can even spin up the devtools and iterate live over the design (we do have the experimental ui-devtools [1] for views, but it has only very little). mus+ash will need some way to show webui regardless of how KSV works (and we can delegate to chrome until it's done; but see more below). So I don't see that as a big issue, in the sense that KSV does not make this any more difficult. While I am here, there's one other thing I wanted to mention :) (sorry!) : as you are well aware, when we first started doing mus+ash a few years back, we starting splitting up ash into smaller separate apps: wallpaper, shelf, etc. We have gone back to having a single ash again, but I think we should still look into splitting some parts of ash out into their own components. Especially for features that are not very often used, does not lie in the critical path of the majority of the interaction. Some good examples: touch_hud (done: [2]), autoclick (done: [3]), magnifier etc. Refactoring existing code so that it works in two different modes is a little bit difficult. So we didn't do others at the time (e.g. magnifier, I think laser/fast_ink etc. should also be separate from ash, etc.). But this should be much easier to do for something completely new. KSV fits the bill of being fairly isolated, off the critical path, and not used often (and also: not super urgent; we will probably be OK if the release gets delayed by one M if needed for some of the extra work?). I think this would be a really good candidate for being one of the first pure mus-apps we launch, and be a good playground for experimenting with how such mus-apps can work, can be organized, how much startup latency we can expect, how to make them better etc. [1] https://www.chromium.org/developers/how-tos/inspecting-ash [2] https://cs.chromium.org/chromium/src/ash/touch_hud/mus/BUILD.gn?l=35 [3] https://cs.chromium.org/chromium/src/ash/autoclick/mus/BUILD.gn?l=37
,
Jan 17 2018
Uploaded a cl to implement the general side tabs with current styling. https://chromium-review.googlesource.com/c/chromium/src/+/872050 Attached please find some images of the prototypes.
,
Jan 19 2018
Make an adjustment in the cl to make the border looks better: draw left border of the active Tab to be the middle of the left edge of tab_strip and the first letter of Tab text. Please see the attached image.
,
Jan 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a375bb24e5b02adf2986fabc8e3caaec09b959af commit a375bb24e5b02adf2986fabc8e3caaec09b959af Author: wutao <wutao@chromium.org> Date: Sat Jan 20 01:00:56 2018 Add side tabs layout. This cl implements the side tabs layout. Major changes: 1. Allow choosing tab alignment upon init. 2. Support MD for vertical tab alignment. 3. Draw boarder for vertical tab alignment. Bug: 793870 Test: Run in linux build. Screenshots are attached in the bug. Change-Id: Ie39f6693ffb5271388f977e280e03d28c1957db9 Reviewed-on: https://chromium-review.googlesource.com/872050 Reviewed-by: Michael Wasserman <msw@chromium.org> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Commit-Queue: Tao Wu <wutao@chromium.org> Cr-Commit-Position: refs/heads/master@{#530702} [modify] https://crrev.com/a375bb24e5b02adf2986fabc8e3caaec09b959af/ui/views/controls/tabbed_pane/tabbed_pane.cc [modify] https://crrev.com/a375bb24e5b02adf2986fabc8e3caaec09b959af/ui/views/controls/tabbed_pane/tabbed_pane.h [modify] https://crrev.com/a375bb24e5b02adf2986fabc8e3caaec09b959af/ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d32a53a794dc81bf708b46fc794c4dc5cdd72e15 commit d32a53a794dc81bf708b46fc794c4dc5cdd72e15 Author: wutao <wutao@chromium.org> Date: Wed Jan 24 18:57:59 2018 Refine tabbed pane unittest. We are no longer use a native Windows tabbed pane, so this cl changes the comments in the tests and refines the tests. Bug: 793870 Test: Adding and refine TabbedPaneTest.* Change-Id: I26ee6ac8a2249ec1c51294b4bcca14f175ae3b07 Reviewed-on: https://chromium-review.googlesource.com/882204 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Tao Wu <wutao@chromium.org> Cr-Commit-Position: refs/heads/master@{#531627} [modify] https://crrev.com/d32a53a794dc81bf708b46fc794c4dc5cdd72e15/ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc
,
Feb 5 2018
UX specs are available now: https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZaz7gfsqSw6O/files/MCE705zXJbjwgGfJNYI-ttzfwMSJbxAVNpw Will put a cl for the new spec. Attached is a prototype example.
,
Feb 6 2018
Uploading several images to answer some questions for the reviewer.
,
Feb 6 2018
Uploaded more images for final* style:
,
Feb 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2dd92c2170b0ac8f22bcc6e28cbc5e4271acae63 commit 2dd92c2170b0ac8f22bcc6e28cbc5e4271acae63 Author: wutao <wutao@chromium.org> Date: Wed Feb 07 10:01:09 2018 Add side tab style for ChromeOS. This cl adds side tab highlight style for Chrome OS according to UX spec. Changes: 1. No text elide for vertical mode. 2. No MdTab underline animation in vertical && highlight mode. 3. MdTab in vertical mode will not take all available vertical space. Bug: 768932, 793870 Test: TabbedPaneTest.* Change-Id: I7c6b4f06c93e3a4f902caf0c9ce4603ddbccb214 Reviewed-on: https://chromium-review.googlesource.com/902820 Commit-Queue: Tao Wu <wutao@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#534972} [modify] https://crrev.com/2dd92c2170b0ac8f22bcc6e28cbc5e4271acae63/ui/views/controls/tabbed_pane/tabbed_pane.cc [modify] https://crrev.com/2dd92c2170b0ac8f22bcc6e28cbc5e4271acae63/ui/views/controls/tabbed_pane/tabbed_pane.h [modify] https://crrev.com/2dd92c2170b0ac8f22bcc6e28cbc5e4271acae63/ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc
,
Mar 6 2018
,
Aug 28
KSV is shipping, so removing RVG |
||||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||||||||||||
Comment 1 by wutao@chromium.org
, Dec 11 2017