Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2368283002:
views: add focus to TabbedPane (Closed)
|
Created:
4 years, 2 months ago by Elly Fong-Jones Modified:
4 years, 2 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionviews: add focus to TabbedPane
This change:
1) Makes TabbedPane draw a focus ring on the selected tab when the
containing TabbedPane has focus;
2) Adds bindings for left/right arrow keys to switch between tabs
BUG=635176
Committed: https://crrev.com/5974cb6cb046616fdbff93b96cdbdcf9bb5e297e
Cr-Commit-Position: refs/heads/master@{#423961}
Patch Set 1 #
Total comments: 10
Patch Set 2 : don't pass color arg #Patch Set 3 : make Tab focusable #
Total comments: 5
Patch Set 4 : DomKey -> KeyboardCode #
Total comments: 9
Patch Set 5 : remove OnContainerFocusChanged #
Total comments: 10
Patch Set 6 : factor out part of OnStateChanged #Patch Set 7 : move insets #
Total comments: 2
Patch Set 8 : remove MdTabFocusRingBorder #
Total comments: 6
Patch Set 9 : rework focus tests a bit #
Total comments: 2
Patch Set 10 : 2 -> 2.f #
Total comments: 11
Patch Set 11 : sed tr sed #
Total comments: 8
Patch Set 12 : rename some stuff, move Tab class #
Messages
Total messages: 39 (8 generated)
Description was changed from ========== views: add focus to TabbedPane This change: 1) Makes TabbedPane draw a focus ring on the selected tab when the containing TabbedPane has focus; 2) Adds bindings for left/right arrow keys to switch between tabs BUG=635176 ========== to ========== views: add focus to TabbedPane This change: 1) Makes TabbedPane draw a focus ring on the selected tab when the containing TabbedPane has focus; 2) Adds bindings for left/right arrow keys to switch between tabs BUG=635176 ==========
ellyjones@chromium.org changed reviewers: + estade@chromium.org
estade: ptal? :)
estade@chromium.org changed reviewers: + dmazzoni@chromium.org
+dmazzoni for a11y angle https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:63: // This is a bit odd: since Tabs are not actually focusable, but rather the Why make the tabstrip focusable instead of the tabs? I think making the tabs themselves actually focusable would generally be better for accessibility. Also on the topic of a11y, you might need to override GetAccessibleState. https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:277: MdTabFocusRingBorder(SkColor base_color); why pass in a parameter? Why not view.GetNativeTheme()->GetSystemColor in OnPaint? (this also handles a changing native theme) https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:303: bounds.Inset(1, 1); can this instead be stroke width / 2? Makes it more obvious what's going on. https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:319: return gfx::Size(4, 4); Is this necessary? https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:336: int border_thickness = selected() ? 2 : 1; seems a little weird that the border thickness depends on selection state. That doesn't affect layout at all (e.g. make the text jiggle)?
estade, dmazzoni: ptal? estade: I'm still on the fence about making the Tab focusable. I'll try it and see if the behavior is better. https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:63: // This is a bit odd: since Tabs are not actually focusable, but rather the On 2016/09/26 16:28:36, Evan Stade wrote: > Why make the tabstrip focusable instead of the tabs? I think making the tabs > themselves actually focusable would generally be better for accessibility. Also > on the topic of a11y, you might need to override GetAccessibleState. I thought it would end up being sort of semantically weird to have the Tab focused, but the left/right key bindings operating at the level of the TabStrip. Neither of the approaches is really that pleasing to me, I guess. You have a good point about accessibility focus, though, so perhaps I do need to rework this to focus the Tab. https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:277: MdTabFocusRingBorder(SkColor base_color); On 2016/09/26 16:28:35, Evan Stade wrote: > why pass in a parameter? Why not view.GetNativeTheme()->GetSystemColor in > OnPaint? (this also handles a changing native theme) Oh good call, reaching into the View did not occur to me :) https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:303: bounds.Inset(1, 1); On 2016/09/26 16:28:36, Evan Stade wrote: > can this instead be stroke width / 2? Makes it more obvious what's going on. Done. https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:319: return gfx::Size(4, 4); On 2016/09/26 16:28:35, Evan Stade wrote: > Is this necessary? It seems to be - GetMinimumSize() is pure virtual on Border, so we have to implement it here. I changed it to just use the insets though. https://codereview.chromium.org/2368283002/diff/1/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane.cc:336: int border_thickness = selected() ? 2 : 1; On 2016/09/26 16:28:35, Evan Stade wrote: > seems a little weird that the border thickness depends on selection state. That > doesn't affect layout at all (e.g. make the text jiggle)? It does not seem to. It *is* a little weird - maybe the right approach is for it to always be 2pt, but either draw 2pt of FocusedBorderColor or 1pt or transparent and 1pt of UnfocusedBorderColor?
It might be slightly easier to code if the tab has focus instead of the tab strip. The user experience is the same either way; they see that a tab has focus but it's part of a larger container so the arrows let you change which one is focused and selected. Radio buttons work the same way - only the active one gets focused, and arrow keys change the selected one. It's definitely possible to make everything work if the container is the one that has focus, but to get screen reader support you'd need to implement "active descendant", which I don't think we've ever needed to plumb through for views. (We use it on the web all the time.) If focus is on the element within the container and the container is the parent, then screen reader support usually just works.
On 2016/09/26 17:58:06, dmazzoni wrote: > It might be slightly easier to code if the tab has focus instead > of the tab strip. The user experience is the same either way; they > see that a tab has focus but it's part of a larger container so > the arrows let you change which one is focused and selected. > > Radio buttons work the same way - only the active one gets > focused, and arrow keys change the selected one. > > It's definitely possible to make everything work if the container > is the one that has focus, but to get screen reader support you'd > need to implement "active descendant", which I don't think we've > ever needed to plumb through for views. (We use it on the web all > the time.) If focus is on the element within the container and the > container is the parent, then screen reader support usually just > works. Okay, I've changed this to give focus to the Tab rather than the TabbedPane. Practically, this is a little messy, because only the selected tab can be marked as focusable (otherwise the tab key will traverse the tabstrip, with the effect of swapping the tab selection) but it seems to work anyway. Thoughts?
On 2016/09/27 at 14:48:50, ellyjones wrote: > Okay, I've changed this to give focus to the Tab rather than the TabbedPane. Practically, this is a little messy, because only the selected tab can be marked as focusable (otherwise the tab key will traverse the tabstrip, with the effect of swapping the tab selection) but it seems to work anyway. Thoughts? Some UI toolkits draw a distinction between "focusable" and "focusable and part of the tab order" for this reason, like tabIndex=0 vs tabIndex=-1 on the web. I wonder if Views should have something like that.
lg for accessibility, I'll leave it to Evan to review
https://codereview.chromium.org/2368283002/diff/40001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/40001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:515: bool give_focus = false; I'm a bit confused here. Shouldn't left and right move focus without changing selection? Selection would change after you press enter or space or something. The tabs should handle left/right arrow events with GetFocusManager()->AdvanceFocus(). Also, maybe all the tabs should go in one group (see IsGroupFocusTraversable()) https://codereview.chromium.org/2368283002/diff/40001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:605: ui::DomKey key = event.GetDomKey(); I don't know what DomKey is, what I've seen more often is event.key_code() == VKEY_LEFT. I don't know which is more correct but codesearch shows an order of magnitude more hits for key_code()
https://codereview.chromium.org/2368283002/diff/40001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/40001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:515: bool give_focus = false; On 2016/09/27 17:19:55, Evan Stade wrote: > I'm a bit confused here. Shouldn't left and right move focus without changing > selection? Selection would change after you press enter or space or something. Native tab controls on both Mac and Windows change both focus and selection when you press arrow keys, we should probably behave the same way.
estade: ptal? :) dmazzoni: do you have any a11y concerns now that the tab is focused? https://codereview.chromium.org/2368283002/diff/40001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/40001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:515: bool give_focus = false; On 2016/09/27 17:46:59, dmazzoni wrote: > On 2016/09/27 17:19:55, Evan Stade wrote: > > I'm a bit confused here. Shouldn't left and right move focus without changing > > selection? Selection would change after you press enter or space or something. > > Native tab controls on both Mac and Windows change both focus and selection when > you press arrow keys, we should probably behave the same way. Yep, this is matching the behavior of the native tabbed pane on Mac. https://codereview.chromium.org/2368283002/diff/40001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:605: ui::DomKey key = event.GetDomKey(); On 2016/09/27 17:19:55, Evan Stade wrote: > I don't know what DomKey is, what I've seen more often is event.key_code() == > VKEY_LEFT. I don't know which is more correct but codesearch shows an order of > magnitude more hits for key_code() Hm, yeah. I don't know what DomKey is either, it was just the first thing I saw that looked like what I wanted :) fixed.
https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:66: void OnContainerFocusChanged(); this doesn't seem necessary any more? At least the comment is no longer true (tabs are focusable). https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:515: bool give_focus = false; nit: declare after early return https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:519: if (GetSelectedTab()) { can we move this below the tab->SetSelected call below so we don't have give_focus? https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:572: next_selected_index += tab_count; nit: I think you can just add tab_count in with delta (i.e. before mod division) to save a line.
estade: ptal? :) https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:66: void OnContainerFocusChanged(); On 2016/09/28 17:32:32, Evan Stade wrote: > this doesn't seem necessary any more? At least the comment is no longer true > (tabs are focusable). Done. https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:515: bool give_focus = false; On 2016/09/28 17:32:32, Evan Stade wrote: > nit: declare after early return Done. https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:519: if (GetSelectedTab()) { On 2016/09/28 17:32:32, Evan Stade wrote: > can we move this below the tab->SetSelected call below so we don't have > give_focus? Ooh, yes, good thinking! Done. https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:572: next_selected_index += tab_count; On 2016/09/28 17:32:32, Evan Stade wrote: > nit: I think you can just add tab_count in with delta (i.e. before mod division) > to save a line. I think the reason this code is done this way is that if you supply a very large delta (like, -1000), (selected_tab_index() + delta) < 0, and so (selected_tab_index() + delta) % tab_count < 0, and also (selected_tab_index() + delta + tab_count) % tab_count < 0, but (selected_tab_index() + delta) % tab_count + tab_count is never negative. We could save the line here, but at the cost of the code crashing if someone supplies a large negative delta in the future. All considered I think I prefer the extra line.
https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:572: next_selected_index += tab_count; On 2016/09/28 18:01:01, Elly Fong-Jones wrote: > On 2016/09/28 17:32:32, Evan Stade wrote: > > nit: I think you can just add tab_count in with delta (i.e. before mod > division) > > to save a line. > > I think the reason this code is done this way is that if you supply a very large > delta (like, -1000), (selected_tab_index() + delta) < 0, and so > (selected_tab_index() + delta) % tab_count < 0, and also (selected_tab_index() + > delta + tab_count) % tab_count < 0, but (selected_tab_index() + delta) % > tab_count + tab_count is never negative. We could save the line here, but at the > cost of the code crashing if someone supplies a large negative delta in the > future. All considered I think I prefer the extra line. good point https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:181: SetFocusBehavior(selected ? FocusBehavior::ALWAYS : FocusBehavior::NEVER); Button::SetFocusForPlatform has to do something similar. It seems like this is a pretty clunky little stanza of code here, can we streamline it? What if we created a new value of FocusBehavior which acted like ACCESSIBLE_ONLY on mac and ALWAYS elsewhere? https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:312: gfx::Rect bounds = gfx::Rect(0, 0, view.width(), view.height()); this is equivalent to view.GetLocalBounds() https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:325: return gfx::Insets(2, 2); this 2 is the same as kBorderStrokeWidth, no? also, I think you can just do gfx::Insets(kBorderStrokeWidth) (there's a ctor for that) https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:348: SetBorder(base::MakeUnique<MdTabFocusRingBorder>()); Can we just always use MdTabFocusRingBorder? In the OnPaint function, check view.HasFocus() and tab_->selected() (you would have to pass an MdTab into the MdTabFocusRingBorder ctor) this actually makes me think we should just override View::OnDrawBorder in MdTab. The separate border class isn't buying us much. (The reason that Border exists as a separate class is to share border code between views, but I don't think we plan to share this one any time soon.)
On Wed, Sep 28, 2016 at 10:21 AM <ellyjones@chromium.org> wrote: > estade: ptal? :) > dmazzoni: do you have any a11y concerns now that the tab is focused? > No concerns. lgtm -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
estade: ptal? :) https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:181: SetFocusBehavior(selected ? FocusBehavior::ALWAYS : FocusBehavior::NEVER); On 2016/09/28 21:25:54, Evan Stade wrote: > Button::SetFocusForPlatform has to do something similar. It seems like this is a > pretty clunky little stanza of code here, can we streamline it? What if we > created a new value of FocusBehavior which acted like ACCESSIBLE_ONLY on mac and > ALWAYS elsewhere? From a cursory look, it seems that ACCESSIBLE_ONLY and ALWAYS are synonyms on non-Mac anyway, so in principle it'd be safe to always use the OS_MACOSX code here. I've contacted karandeepb@ (who originally introduced ACCESSIBLE_ONLY) to ask if there's some reason we shouldn't simply do that. https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:312: gfx::Rect bounds = gfx::Rect(0, 0, view.width(), view.height()); On 2016/09/28 21:25:54, Evan Stade wrote: > this is equivalent to view.GetLocalBounds() Done. https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:325: return gfx::Insets(2, 2); On 2016/09/28 21:25:54, Evan Stade wrote: > this 2 is the same as kBorderStrokeWidth, no? > > also, I think you can just do gfx::Insets(kBorderStrokeWidth) (there's a ctor > for that) Done. https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:348: SetBorder(base::MakeUnique<MdTabFocusRingBorder>()); On 2016/09/28 21:25:54, Evan Stade wrote: > Can we just always use MdTabFocusRingBorder? In the OnPaint function, check > view.HasFocus() and tab_->selected() (you would have to pass an MdTab into the > MdTabFocusRingBorder ctor) > > this actually makes me think we should just override View::OnDrawBorder in > MdTab. The separate border class isn't buying us much. (The reason that Border > exists as a separate class is to share border code between views, but I don't > think we plan to share this one any time soon.) I moved all this logic into MdTabFocusRingBorder::Paint(), so this function is now quite short. I don't really want to override OnPaintBorder, though - it seems kind of icky to circumvent Views' builtin border drawing. If you feel strongly I can change it, but for now I've left MdTabFocusRingBorder as-is.
https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:181: SetFocusBehavior(selected ? FocusBehavior::ALWAYS : FocusBehavior::NEVER); On 2016/09/30 11:54:07, Elly Fong-Jones wrote: > On 2016/09/28 21:25:54, Evan Stade wrote: > > Button::SetFocusForPlatform has to do something similar. It seems like this is > a > > pretty clunky little stanza of code here, can we streamline it? What if we > > created a new value of FocusBehavior which acted like ACCESSIBLE_ONLY on mac > and > > ALWAYS elsewhere? > > From a cursory look, it seems that ACCESSIBLE_ONLY and ALWAYS are synonyms on > non-Mac anyway, so in principle it'd be safe to always use the OS_MACOSX code > here. I've contacted karandeepb@ (who originally introduced ACCESSIBLE_ONLY) to > ask if there's some reason we shouldn't simply do that. hmm, don't you mean to always use the non-osx code here? https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:348: SetBorder(base::MakeUnique<MdTabFocusRingBorder>()); On 2016/09/30 11:54:07, Elly Fong-Jones wrote: > On 2016/09/28 21:25:54, Evan Stade wrote: > > Can we just always use MdTabFocusRingBorder? In the OnPaint function, check > > view.HasFocus() and tab_->selected() (you would have to pass an MdTab into the > > MdTabFocusRingBorder ctor) > > > > this actually makes me think we should just override View::OnDrawBorder in > > MdTab. The separate border class isn't buying us much. (The reason that Border > > exists as a separate class is to share border code between views, but I don't > > think we plan to share this one any time soon.) > > I moved all this logic into MdTabFocusRingBorder::Paint(), so this function is > now quite short. I don't really want to override OnPaintBorder, though - it > seems kind of icky to circumvent Views' builtin border drawing. If you feel > strongly I can change it, but for now I've left MdTabFocusRingBorder as-is. why is it icky? OnPaintBorder wouldn't be virtual if it weren't meant to be overridden. https://codereview.chromium.org/2368283002/diff/120001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/120001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:287: MdTabFocusRingBorder(MdTab* tab); passing in the tab is what makes me think the border drawing is really a property of the tab itself. I still don't think a separate border class buys us anything here. If you were to keep this, the ctor should be marked explicit.
On 2016/09/30 17:24:42, Evan Stade wrote: > https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... > File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): > > https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... > ui/views/controls/tabbed_pane/tabbed_pane.cc:181: SetFocusBehavior(selected ? > FocusBehavior::ALWAYS : FocusBehavior::NEVER); > On 2016/09/30 11:54:07, Elly Fong-Jones wrote: > > On 2016/09/28 21:25:54, Evan Stade wrote: > > > Button::SetFocusForPlatform has to do something similar. It seems like this > is > > a > > > pretty clunky little stanza of code here, can we streamline it? What if we > > > created a new value of FocusBehavior which acted like ACCESSIBLE_ONLY on mac > > and > > > ALWAYS elsewhere? > > > > From a cursory look, it seems that ACCESSIBLE_ONLY and ALWAYS are synonyms on > > non-Mac anyway, so in principle it'd be safe to always use the OS_MACOSX code > > here. I've contacted karandeepb@ (who originally introduced ACCESSIBLE_ONLY) > to > > ask if there's some reason we shouldn't simply do that. > > hmm, don't you mean to always use the non-osx code here? I meant the OSX code, since ACCESSIBLE_ONLY would work on every platform, but I checked with karandeepb@ and ACCESSIBLE_ONLY and ALWAYS aren't actually synonyms on Windows either :( I'll put up a CL after this one to factor this code snippet out at least. > https://codereview.chromium.org/2368283002/diff/80001/ui/views/controls/tabbe... > ui/views/controls/tabbed_pane/tabbed_pane.cc:348: > SetBorder(base::MakeUnique<MdTabFocusRingBorder>()); > On 2016/09/30 11:54:07, Elly Fong-Jones wrote: > > On 2016/09/28 21:25:54, Evan Stade wrote: > > > Can we just always use MdTabFocusRingBorder? In the OnPaint function, check > > > view.HasFocus() and tab_->selected() (you would have to pass an MdTab into > the > > > MdTabFocusRingBorder ctor) > > > > > > this actually makes me think we should just override View::OnDrawBorder in > > > MdTab. The separate border class isn't buying us much. (The reason that > Border > > > exists as a separate class is to share border code between views, but I > don't > > > think we plan to share this one any time soon.) > > > > I moved all this logic into MdTabFocusRingBorder::Paint(), so this function is > > now quite short. I don't really want to override OnPaintBorder, though - it > > seems kind of icky to circumvent Views' builtin border drawing. If you feel > > strongly I can change it, but for now I've left MdTabFocusRingBorder as-is. > > why is it icky? OnPaintBorder wouldn't be virtual if it weren't meant to be > overridden. Okay. I moved the border painting into OnPaintBorder. > https://codereview.chromium.org/2368283002/diff/120001/ui/views/controls/tabb... > File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): > > https://codereview.chromium.org/2368283002/diff/120001/ui/views/controls/tabb... > ui/views/controls/tabbed_pane/tabbed_pane.cc:287: MdTabFocusRingBorder(MdTab* > tab); > passing in the tab is what makes me think the border drawing is really a > property of the tab itself. I still don't think a separate border class buys us > anything here. > > If you were to keep this, the ctor should be marked explicit. I ended up removing it.
estade, sky: ptal? :) https://codereview.chromium.org/2368283002/diff/120001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/120001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:287: MdTabFocusRingBorder(MdTab* tab); On 2016/09/30 17:24:41, Evan Stade wrote: > passing in the tab is what makes me think the border drawing is really a > property of the tab itself. I still don't think a separate border class buys us > anything here. > > If you were to keep this, the ctor should be marked explicit. Done.
https://codereview.chromium.org/2368283002/diff/140001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/140001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:337: bounds.Inset(kBorderStrokeWidth / 2, kBorderStrokeWidth / 2); nit: can be bounds.Inset(gfx::Insets(kBorderStrokeWidth / 2)); https://codereview.chromium.org/2368283002/diff/140001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:342: canvas->DrawRect(bounds, paint); this version of DrawRect is deprecated (see canvas.h) https://codereview.chromium.org/2368283002/diff/140001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:488: if (GetSelectedTab()) { this is confusing on a couple levels: a) "tab" is too generic of a name when you're dealing with multiple Tabs b) you set tab to selected, but GetSelectedTab still returns the old selected tab. Can you do something like: Tab* current_selected_tab = GetSelectedTab(); Tab* new_selected_tab = GetTabAt(...); ...
estade, sky: ptal? :) https://codereview.chromium.org/2368283002/diff/140001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/140001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:337: bounds.Inset(kBorderStrokeWidth / 2, kBorderStrokeWidth / 2); On 2016/10/03 19:15:16, Evan Stade wrote: > nit: can be > > bounds.Inset(gfx::Insets(kBorderStrokeWidth / 2)); Done. https://codereview.chromium.org/2368283002/diff/140001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:342: canvas->DrawRect(bounds, paint); On 2016/10/03 19:15:15, Evan Stade wrote: > this version of DrawRect is deprecated (see canvas.h) Done. https://codereview.chromium.org/2368283002/diff/140001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:488: if (GetSelectedTab()) { On 2016/10/03 19:15:15, Evan Stade wrote: > this is confusing on a couple levels: > > a) "tab" is too generic of a name when you're dealing with multiple Tabs > b) you set tab to selected, but GetSelectedTab still returns the old selected > tab. > > Can you do something like: > Tab* current_selected_tab = GetSelectedTab(); > Tab* new_selected_tab = GetTabAt(...); > ... Done.
lgtm https://codereview.chromium.org/2368283002/diff/160001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/160001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:336: bounds.Inset(gfx::Insets(kBorderStrokeWidth / 2)); nit: gfx::InsetsF, and s/2/2.f/
ellyjones@chromium.org changed reviewers: + sky@chromium.org
sky: ptal? :) https://codereview.chromium.org/2368283002/diff/160001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/160001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:336: bounds.Inset(gfx::Insets(kBorderStrokeWidth / 2)); On 2016/10/04 21:12:10, Evan Stade wrote: > nit: gfx::InsetsF, and s/2/2.f/ Done.
https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:276: contents()->NotifyAccessibilityEvent(ui::AX_EVENT_FOCUS, true); How come we only call NotifyAccessibilityEvent() in OnFocus and not OnBlur? Maybe a comment. https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:475: SelectTabAt(index); In looking at this code again it seems there should be an else case to potentially update selected_tab_index_ if selected_tab_index >= index. Alternatively we could get rid of selected_tab_index_ and query the tabs directly. I think it's more important with your change to keep things in sync, but feel free to add a TODO and come back to this. https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:566: bool TabbedPane::OnKeyPressed(const ui::KeyEvent& event) { How about test coverage of pressing left/right? https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (right): https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:58: View* GetSelectedTabViewForTesting(); I'm not a fan of ForTesting public functions. They clutter the API. Can you friend the test? https://codereview.chromium.org/2368283002/diff/180001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2368283002/diff/180001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:36: enum { enum style for chrome is ALL_CAPS.
sky: ptal? :) https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:276: contents()->NotifyAccessibilityEvent(ui::AX_EVENT_FOCUS, true); On 2016/10/05 20:00:22, sky wrote: > How come we only call NotifyAccessibilityEvent() in OnFocus and not OnBlur? > Maybe a comment. Done. https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:475: SelectTabAt(index); On 2016/10/05 20:00:22, sky wrote: > In looking at this code again it seems there should be an else case to > potentially update selected_tab_index_ if selected_tab_index >= index. > Alternatively we could get rid of selected_tab_index_ and query the tabs > directly. I think it's more important with your change to keep things in sync, > but feel free to add a TODO and come back to this. Agreed; added a TODO. https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:566: bool TabbedPane::OnKeyPressed(const ui::KeyEvent& event) { On 2016/10/05 20:00:22, sky wrote: > How about test coverage of pressing left/right? Done. https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (right): https://codereview.chromium.org/2368283002/diff/180001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:58: View* GetSelectedTabViewForTesting(); On 2016/10/05 20:00:22, sky wrote: > I'm not a fan of ForTesting public functions. They clutter the API. Can you > friend the test? Done. https://codereview.chromium.org/2368283002/diff/180001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2368283002/diff/180001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:36: enum { On 2016/10/05 20:00:22, sky wrote: > enum style for chrome is ALL_CAPS. This is done... but at what cost??
https://codereview.chromium.org/2368283002/diff/180001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2368283002/diff/180001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:36: enum { On 2016/10/06 16:15:30, Elly Fong-Jones wrote: > On 2016/10/05 20:00:22, sky wrote: > > enum style for chrome is ALL_CAPS. > > This is done... but at what cost?? Consistency? https://codereview.chromium.org/2368283002/diff/200001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (right): https://codereview.chromium.org/2368283002/diff/200001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:71: View* GetSelectedTabContentView(); Ick! Why not move the declaration of tab to this header so you don't weirdly duplicated functions? https://codereview.chromium.org/2368283002/diff/200001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://codereview.chromium.org/2368283002/diff/200001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:105: EXPECT_EQ(0, tabbed_pane->selected_tab_index()); Do you really need to do this assertion every test? I would be inclined to move it outside the loop.
naming nits :) https://codereview.chromium.org/2368283002/diff/200001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2368283002/diff/200001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:71: O_K_BUTTON_ID, heh^ https://codereview.chromium.org/2368283002/diff/200001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:77: ITALIC_CHECK_BOX_ID, s/CHECK_BOX/CHECKBOX here and everywhere
sky: ptal? :) https://codereview.chromium.org/2368283002/diff/200001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (right): https://codereview.chromium.org/2368283002/diff/200001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:71: View* GetSelectedTabContentView(); On 2016/10/06 21:14:40, sky wrote: > Ick! Why not move the declaration of tab to this header so you don't weirdly > duplicated functions? Done. https://codereview.chromium.org/2368283002/diff/200001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://codereview.chromium.org/2368283002/diff/200001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:105: EXPECT_EQ(0, tabbed_pane->selected_tab_index()); On 2016/10/06 21:14:40, sky wrote: > Do you really need to do this assertion every test? I would be inclined to move > it outside the loop. No, I don't; it was just cargo-culted off one of the tests above in this file. Moved it. https://codereview.chromium.org/2368283002/diff/200001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2368283002/diff/200001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:71: O_K_BUTTON_ID, On 2016/10/06 21:18:44, Evan Stade (ooo friday) wrote: > heh^ Fixed. I'm still feeling clever that I automated this rename :) https://codereview.chromium.org/2368283002/diff/200001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:77: ITALIC_CHECK_BOX_ID, On 2016/10/06 21:18:44, Evan Stade (ooo friday) wrote: > s/CHECK_BOX/CHECKBOX here and everywhere Done.
LGTM
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org Link to the patchset: https://codereview.chromium.org/2368283002/#ps220001 (title: "rename some stuff, move Tab class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== views: add focus to TabbedPane This change: 1) Makes TabbedPane draw a focus ring on the selected tab when the containing TabbedPane has focus; 2) Adds bindings for left/right arrow keys to switch between tabs BUG=635176 ========== to ========== views: add focus to TabbedPane This change: 1) Makes TabbedPane draw a focus ring on the selected tab when the containing TabbedPane has focus; 2) Adds bindings for left/right arrow keys to switch between tabs BUG=635176 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== views: add focus to TabbedPane This change: 1) Makes TabbedPane draw a focus ring on the selected tab when the containing TabbedPane has focus; 2) Adds bindings for left/right arrow keys to switch between tabs BUG=635176 ========== to ========== views: add focus to TabbedPane This change: 1) Makes TabbedPane draw a focus ring on the selected tab when the containing TabbedPane has focus; 2) Adds bindings for left/right arrow keys to switch between tabs BUG=635176 Committed: https://crrev.com/5974cb6cb046616fdbff93b96cdbdcf9bb5e297e Cr-Commit-Position: refs/heads/master@{#423961} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/5974cb6cb046616fdbff93b96cdbdcf9bb5e297e Cr-Commit-Position: refs/heads/master@{#423961} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
