Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
333010:
Implement keyboard access between bookmarks and toolbar. (Closed)
|
Created:
11 years, 2 months ago by Mohamed Mansour Modified:
9 years, 6 months ago CC:
chromium-reviews_googlegroups.com Visibility:
Public. |
DescriptionImplement keyboard access between bookmarks and main toolbar.
Allow ALT+SHIFT+T and TAB to traverse between bookmarks bar and main toolbar, same thing goes for SHIFT+TAB. Once any toolbar is focused, the arrow keys are used to traverse its children views.
Any toolbar that needs to be traversable needs to extend "AccessibleToolbarView", that class will deal with all the toolbar accessibility needs such as handling ESC, Left/Right arrows, Enter, Drop downs, and traversals with Tab/Shift+Tab.
There is one abstract method that the views who are extending this would need to implement if needed which allows the toolbar to skip views that are being traversed:
bool IsAccessibleViewTraversable(views::View* view)
BUG=25625
TEST=Test to see if the main toolbar and bookmarks bar is traversable.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33793
Patch Set 1 : Fix headers for Linux compilation #
Total comments: 40
Patch Set 2 : Fix nits and functionality #
Total comments: 32
Patch Set 3 : Complete reorganization, much cleaner and better! #
Total comments: 3
Patch Set 4 : rebase #
Total comments: 30
Patch Set 5 : Fix nits #Patch Set 6 : Minor change #
Total comments: 6
Patch Set 7 : Attempt to address the sky #Patch Set 8 : rebase #Patch Set 9 : Save the original last focused view so pressing ESC would work with multiple toolbars #
Total comments: 4
Patch Set 10 : Apply Scott's changes #
Total comments: 4
Patch Set 11 : Fix GYP nits #
Total comments: 8
Patch Set 12 : Add more null checks #
Messages
Total messages: 23 (0 generated)
This is just an initial implementation on what I think could be a possible solution. I don't like my naming, so I will change them tomorrow. If there are any comments on how I subclassed it, please let me know! I will try and finish this before Jonas goes on vacation! There are some questions for Jay, regarding how we traverse between toolbars. I already hooked the TAB+SHIFT and TAB to traverse but what would be the best way to know the order of traversing. I was thinking of statically specifying the order in BrowserView (via some array of AccessibleToolbarView) and have a counter that tells me which index I am currently traversing. I will complete this tomorrow. Just to let you guys know what I am doing.
It works, yay. So far, just Bookmarks bar and Toolbar bar is traversable. Please review!
Anyone willing to review please? This does some work on "views" so that the toolbars could be accessible via keyboard.
http://codereview.chromium.org/333010/diff/3002/2022 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/3002/2022#newcode34 Line 34: // Skip any views that cannot be interacted with. This loop is hard to read. You should invert your if so that if it's true, you break, eg: if (IsNextAccessibleViewTraversable(current_view_index) &&...) break; Also, you may as well return from here rather than breaking. http://codereview.chromium.org/333010/diff/3002/2022#newcode35 Line 35: if (!IsNextAccessibleViewTraversable(current_view_index) || You should check enabled/visible first, then call the virtual method. http://codereview.chromium.org/333010/diff/3002/2022#newcode61 Line 61: // HACK: Do not use RequestFocus() here, as the toolbar is not marked as Is this comment still correct? Seems like your override of RequestFocus makes this work. http://codereview.chromium.org/333010/diff/3002/2022#newcode92 Line 92: int view_index = VIEW_ID_TOOLBAR; view_id http://codereview.chromium.org/333010/diff/3002/2022#newcode153 Line 153: if (!acc_focused_view_) Shouldn't this handle selecting a view if nothing was selected? http://codereview.chromium.org/333010/diff/3002/2022#newcode168 Line 168: if (OnAccessibleKeyPressed()) break; Views that care about this should override OnKeyPressed and do the check directly rather than having it here. http://codereview.chromium.org/333010/diff/3002/2022#newcode175 Line 175: if (next_view == -1) || next_view == focused_view http://codereview.chromium.org/333010/diff/3002/2022#newcode179 Line 179: if (next_view != focused_view) { The code in this branch is nearly the same as that in DidGainFocus. You should have a single place that has all this code. http://codereview.chromium.org/333010/diff/3002/2022#newcode257 Line 257: (*name).assign(accessible_name_); *name = accessible_name_; return !accessible_name_.empty(); http://codereview.chromium.org/333010/diff/3002/2022#newcode271 Line 271: accessible_name_.assign(name); accessible_name_ = name; http://codereview.chromium.org/333010/diff/3002/2023 File chrome/browser/views/accessible_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/3002/2023#newcode8 Line 8: #include "chrome/browser/views/frame/browser_view.h" Forward declare BrowserView rather than including. http://codereview.chromium.org/333010/diff/3002/2023#newcode11 Line 11: // If a toolbar needs to be traversal then it needs to be focusable. But we do The description should start with what this class does and not a description of focus, eg: This class provides keywboard access to any view that extends it... The description should also mention it traverses child views in order, which is not necessarily visual order. http://codereview.chromium.org/333010/diff/3002/2023#newcode19 Line 19: explicit AccessibleToolbarView(BrowserView* parent); I don't like everyone having to take BrowserView here. It would be better to dig out when needed by way of looking for a view with a particular id. http://codereview.chromium.org/333010/diff/3002/2023#newcode23 Line 23: // view index, in the given navigation direction (nav_left true means nav_left is a bit confusing here. Can you rename it forward instead. http://codereview.chromium.org/333010/diff/3002/2023#newcode29 Line 29: void InitializeTraversal(); Is there a reason we don't want this to be RequestFocus? http://codereview.chromium.org/333010/diff/3002/2023#newcode42 Line 42: virtual bool IsNextAccessibleViewTraversable(int current_view_index); By naming this 'Next' it reads as though it should return the value current_view_index +1/-1, which isn't the case. You should nuke the Next. http://codereview.chromium.org/333010/diff/3002/2023#newcode61 Line 61: You need to deal with the currently focused accessible view getting removed out from under you. http://codereview.chromium.org/333010/diff/3002/2024 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/333010/diff/3002/2024#newcode705 Line 705: GetWidget()->GetTooltipManager()->HideKeyboardTooltip(); This seems like a separate issue. Any time a menu is shown tooltips should be hidden. http://codereview.chromium.org/333010/diff/3002/2024#newcode815 Line 815: other_bookmarked_button_ = CreateOtherBookmarkedButton(); Notice the order the buttons are added in isn't the order you want the buttons traversed. http://codereview.chromium.org/333010/diff/3002/2031 File chrome/browser/views/frame/browser_view.h (right): http://codereview.chromium.org/333010/diff/3002/2031#newcode34 Line 34: class AccessibleToolbarView; As you don't use AccessibleToolbarView in the header, you shouldn't need to forward declare it.
Thanks for the review. Most of the code in accessible_toolbar_view came from (moved) from toolbar_view. Alot of those nits is cleaning up existing code, which is good to do :) I applied my changes with some questions. jcampan any suggestions with the focus stuff? http://codereview.chromium.org/333010/diff/3002/2022 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/3002/2022#newcode34 Line 34: // Skip any views that cannot be interacted with. On 2009/10/27 21:40:12, sky wrote: > This loop is hard to read. You should invert your if so that if it's true, you > break, eg: > > if (IsNextAccessibleViewTraversable(current_view_index) &&...) > break; > > Also, you may as well return from here rather than breaking. Done. http://codereview.chromium.org/333010/diff/3002/2022#newcode35 Line 35: if (!IsNextAccessibleViewTraversable(current_view_index) || On 2009/10/27 21:40:12, sky wrote: > You should check enabled/visible first, then call the virtual method. Done. http://codereview.chromium.org/333010/diff/3002/2022#newcode61 Line 61: // HACK: Do not use RequestFocus() here, as the toolbar is not marked as On 2009/10/27 21:40:12, sky wrote: > Is this comment still correct? Seems like your override of RequestFocus makes > this work. Done. I realized that your right :) thanks. http://codereview.chromium.org/333010/diff/3002/2022#newcode92 Line 92: int view_index = VIEW_ID_TOOLBAR; On 2009/10/27 21:40:12, sky wrote: > view_id Done. http://codereview.chromium.org/333010/diff/3002/2022#newcode153 Line 153: if (!acc_focused_view_) On 2009/10/27 21:40:12, sky wrote: > Shouldn't this handle selecting a view if nothing was selected? Done. Correct! http://codereview.chromium.org/333010/diff/3002/2022#newcode168 Line 168: if (OnAccessibleKeyPressed()) break; On 2009/10/27 21:40:12, sky wrote: > Views that care about this should override OnKeyPressed and do the check > directly rather than having it here. Done. Yea, so true, makes it cleaner, then the view will call super.OnKeyPressed(evt) http://codereview.chromium.org/333010/diff/3002/2022#newcode175 Line 175: if (next_view == -1) On 2009/10/27 21:40:12, sky wrote: > || next_view == focused_view Done. http://codereview.chromium.org/333010/diff/3002/2022#newcode179 Line 179: if (next_view != focused_view) { On 2009/10/27 21:40:12, sky wrote: > The code in this branch is nearly the same as that in DidGainFocus. You should > have a single place that has all this code. Its late right now (2AM) I will do this tomorrow evening! The stuff I could share is almost everything under. I will make it a private method and name it FocusAccessibleView() http://codereview.chromium.org/333010/diff/3002/2022#newcode257 Line 257: (*name).assign(accessible_name_); On 2009/10/27 21:40:12, sky wrote: > *name = accessible_name_; > return !accessible_name_.empty(); Done. http://codereview.chromium.org/333010/diff/3002/2022#newcode271 Line 271: accessible_name_.assign(name); On 2009/10/27 21:40:12, sky wrote: > accessible_name_ = name; Done. http://codereview.chromium.org/333010/diff/3002/2023 File chrome/browser/views/accessible_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/3002/2023#newcode8 Line 8: #include "chrome/browser/views/frame/browser_view.h" On 2009/10/27 21:40:12, sky wrote: > Forward declare BrowserView rather than including. I tried doing that before but I got compilation errors: "error C2027: use of undefined type 'BrowserView' accessible_toolbar_view.cc" http://codereview.chromium.org/333010/diff/3002/2023#newcode11 Line 11: // If a toolbar needs to be traversal then it needs to be focusable. But we do On 2009/10/27 21:40:12, sky wrote: > The description should start with what this class does and not a description of > focus, eg: > > This class provides keywboard access to any view that extends it... > > The description should also mention it traverses child views in order, which is > not necessarily visual order. Done. http://codereview.chromium.org/333010/diff/3002/2023#newcode19 Line 19: explicit AccessibleToolbarView(BrowserView* parent); On 2009/10/27 21:40:12, sky wrote: > I don't like everyone having to take BrowserView here. It would be better to dig > out when needed by way of looking for a view with a particular id. If I would do it that way, then I would have to have another View ID that we call it VIEW_ID_BROWSER_VIEW, right? Then I would have to introduce a recursive function every time I would want to traverse to the next focusable toolbar and same thing goes for focusing on the address bar. The reason it has to be recursive (or a while loop) is because we have to find out which parent is a BrowserView and then static cast it to that type. I am coming from a Java Background, and we use instanceof, but I talked to some chrome people and they don't like static casting :( What do you think the preferred method here should be? I need it call BrowserView::TraverseNextAccessibleToolbar to traverse between toolbars since BrowserView is the hub of all toolbars. http://codereview.chromium.org/333010/diff/3002/2023#newcode23 Line 23: // view index, in the given navigation direction (nav_left true means On 2009/10/27 21:40:12, sky wrote: > nav_left is a bit confusing here. Can you rename it forward instead. Done. http://codereview.chromium.org/333010/diff/3002/2023#newcode29 Line 29: void InitializeTraversal(); On 2009/10/27 21:40:12, sky wrote: > Is there a reason we don't want this to be RequestFocus? Done. Nice catch. http://codereview.chromium.org/333010/diff/3002/2023#newcode42 Line 42: virtual bool IsNextAccessibleViewTraversable(int current_view_index); On 2009/10/27 21:40:12, sky wrote: > By naming this 'Next' it reads as though it should return the value > current_view_index +1/-1, which isn't the case. You should nuke the Next. Done. http://codereview.chromium.org/333010/diff/3002/2023#newcode61 Line 61: On 2009/10/27 21:40:12, sky wrote: > You need to deal with the currently focused accessible view getting removed out > from under you. Why would we need to deal with that? We save the current focused accessible view since that is the only view we care about. We don't care about all the other views. I might be misunderstanding your question. http://codereview.chromium.org/333010/diff/3002/2024 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/333010/diff/3002/2024#newcode705 Line 705: GetWidget()->GetTooltipManager()->HideKeyboardTooltip(); On 2009/10/27 21:40:12, sky wrote: > This seems like a separate issue. Any time a menu is shown tooltips should be > hidden. Done. filed http://crbug.com/26026 http://codereview.chromium.org/333010/diff/3002/2024#newcode815 Line 815: other_bookmarked_button_ = CreateOtherBookmarkedButton(); On 2009/10/27 21:40:12, sky wrote: > Notice the order the buttons are added in isn't the order you want the buttons > traversed. I thought about that too, but we are traversing based on the order of the childviews. In bookmarks bar, it worked great, it traversed the items then the other bookmark, so its valid for this case. I think it is too much overkill to create another focus manager for just each traversal, it works in our cases now, if it doesn't we can make sure the views are in order. http://codereview.chromium.org/333010/diff/3002/2031 File chrome/browser/views/frame/browser_view.h (right): http://codereview.chromium.org/333010/diff/3002/2031#newcode34 Line 34: class AccessibleToolbarView; On 2009/10/27 21:40:12, sky wrote: > As you don't use AccessibleToolbarView in the header, you shouldn't need to > forward declare it. Done.
http://codereview.chromium.org/333010/diff/11002/12002 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/11002/12002#newcode25 Line 25: int modifier = 1; int modified = forward ? -1 : 1; would be simpler http://codereview.chromium.org/333010/diff/11002/12002#newcode34 Line 34: // Try to find the next available view that can be interacted. That view "interacted" -> interacted with http://codereview.chromium.org/333010/diff/11002/12002#newcode44 Line 44: // Mo button is available in the specified direction, remains where it was. Comment wording http://codereview.chromium.org/333010/diff/11002/12002#newcode48 Line 48: bool AccessibleToolbarView::IsAccessibleViewTraversable(int current_view) { Why pass an index and not the view? It seems that it would be more convenient for the subclasses to be passed a view. http://codereview.chromium.org/333010/diff/11002/12002#newcode200 Line 200: // Must have MSAA Focus inorder to handle ESC and TAB related key events. inorder -> in order http://codereview.chromium.org/333010/diff/11002/12002#newcode209 Line 209: // |acc_focused_view_| doesn't need to be resetted here since it will be resetted -> reset http://codereview.chromium.org/333010/diff/11002/12003 File chrome/browser/views/accessible_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/11002/12003#newcode18 Line 18: // tell the focus manager that this view is focusable while its not. The main Nit: its -> it's http://codereview.chromium.org/333010/diff/11002/12003#newcode22 Line 22: class AccessibleToolbarView : public views::View { Insert blank line above. http://codereview.chromium.org/333010/diff/11002/12003#newcode29 Line 29: // will navigate from right to left, and vice versa when false. -1 finds first Did you mean from left to right? http://codereview.chromium.org/333010/diff/11002/12003#newcode38 Line 38: // Set the accessible MSAA focused view. Nit: set -> Sets. http://codereview.chromium.org/333010/diff/11002/12003#newcode60 Line 60: // Child view currently having MSAA focus . MSAA is Windows specific. It should probably replaced with "accessibility". http://codereview.chromium.org/333010/diff/11002/12004 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/333010/diff/11002/12004#newcode709 Line 709: if (GetAccFocusedChildView() == other_bookmarked_button_) { We could have other folders on the bookmark bar. Shouldn't this be generalized to all MenuButtons? http://codereview.chromium.org/333010/diff/11002/12007 File chrome/browser/views/detachable_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/11002/12007#newcode23 Line 23: DetachableToolbarView(BrowserView* parent) : AccessibleToolbarView(parent) {} explicit http://codereview.chromium.org/333010/diff/11002/12009 File chrome/browser/views/extensions/extension_shelf.h (right): http://codereview.chromium.org/333010/diff/11002/12009#newcode31 Line 31: explicit ExtensionShelf(Browser* browser, BrowserView* parent); Remove explicit. http://codereview.chromium.org/333010/diff/11002/12010 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/333010/diff/11002/12010#newcode672 Line 672: toolbar_->RequestFocus(); It seems weird that we initialize the accessiblity traversal by requesting focus. Should we have a StartAccessibilityTraversal method or something? Also it seems |view_index| is not used. http://codereview.chromium.org/333010/diff/11002/12011 File chrome/browser/views/frame/browser_view.h (right): http://codereview.chromium.org/333010/diff/11002/12011#newcode186 Line 186: // given navigation direction. |forward| when true, will navigate from right I find it confusing. Forward means RTL? I would have expected LTR? http://codereview.chromium.org/333010/diff/11002/12013 File chrome/browser/views/toolbar_view.h (right): http://codereview.chromium.org/333010/diff/11002/12013#newcode80 Line 80: explicit ToolbarView(Browser* browser, BrowserView* parent); Remove explicit
> http://codereview.chromium.org/333010/diff/3002/2023 > File chrome/browser/views/accessible_toolbar_view.h (right): > > http://codereview.chromium.org/333010/diff/3002/2023#newcode8 > Line 8: #include "chrome/browser/views/frame/browser_view.h" > On 2009/10/27 21:40:12, sky wrote: >> >> Forward declare BrowserView rather than including. > > I tried doing that before but I got compilation errors: > "error C2027: use of undefined type > 'BrowserView' =A0 accessible_toolbar_view.cc" Did you move the include to the .cc though? > http://codereview.chromium.org/333010/diff/3002/2023#newcode19 > Line 19: explicit AccessibleToolbarView(BrowserView* parent); > On 2009/10/27 21:40:12, sky wrote: >> >> I don't like everyone having to take BrowserView here. It would be > > better to dig >> >> out when needed by way of looking for a view with a particular id. > > If I would do it that way, then I would have to have another View ID > that we call it VIEW_ID_BROWSER_VIEW, right? Yes. > Then I would have to > introduce a recursive function every time I would want to traverse to > the next focusable toolbar and same thing goes for focusing on the > address bar. This method is so rarely used that the extra cost won't be an issue. Additionally you're only walking the parents, and there aren't going to be the many. > The reason it has to be recursive (or a while loop) is because we have > to find out which parent is a BrowserView and then static cast it to > that type. I am coming from a Java Background, and we use instanceof, > but I talked to some chrome people and they don't like static casting :( Static casting in this case is fine. > What do you think the preferred method here should be? I need it call > BrowserView::TraverseNextAccessibleToolbar to traverse between toolbars > since BrowserView is the hub of all toolbars. Sounds good to me. > http://codereview.chromium.org/333010/diff/3002/2023#newcode61 > Line 61: > On 2009/10/27 21:40:12, sky wrote: >> >> You need to deal with the currently focused accessible view getting > > removed out >> >> from under you. > > Why would we need to deal with that? We save the current focused > accessible view since that is the only view we care about. We don't care > about all the other views. I might be misunderstanding your question. Because if the view is removed and you end up invoking anything on it (such as SetHotTracked) we'll crash. > http://codereview.chromium.org/333010/diff/3002/2024#newcode815 > Line 815: other_bookmarked_button_ =3D CreateOtherBookmarkedButton(); > On 2009/10/27 21:40:12, sky wrote: >> >> Notice the order the buttons are added in isn't the order you want the > > buttons >> >> traversed. > > I thought about that too, but we are traversing based on the order of > the childviews. In bookmarks bar, it worked great, it traversed the > items then the other bookmark, so its valid for this case. I think it is > too much overkill to create another focus manager for just each > traversal, it works in our cases now, if it doesn't we can make sure the > views are in order. Did you try this when not all the bookmarks were visible such that you get the overflow button? What is the traversal order like with the overflow button? What about with sync enabled? -Scott
You guys are awesome! Thanks for the reviews =) BTW, this CL is now cleaner, I applied everything you guys have said except one thing. Scott asked me if I could merge InitiateTraversal with RequestFocus since anytime we want to focus a toolbar, it is only possible if we are in accessibility focus mode. But I believe its cleaner to separate them. Your thoughts? Okay, now this CL removes all key events from the toolbars that are subclassing AccessibleToolbarView, and made a generic implementation that checks for MenuButton. I reverted all my constructor changes so it no longer requires BrowserView, I just requested it from the parent (using ancestors) where I needed to, its cleaner and better this way. I moved the "other bookmarks" two views down, since the order of traversing matters. Now we can traverse between the bookmark items, overflow, sync, and other bookmarks correctly. I didn't look into Scott's comment regarding "Because if the view is removed and you end up invoking anything on it (such as SetHotTracked) we'll crash." I will need to figure that out. I refactored some methods in AccessibleToolbarView so it can be reused in numerous places. We only have one abstract method, the others are removed. That abstract method is used if the toolbar wants to skip views while traversing, which is needed. Please review now, and thanks for your time! I am learning a lot from you guys! I love the amount of quality you guys put in reviews. It helps me as a software engineer and helps me try to contribute high quality code back into the source tree! http://codereview.chromium.org/333010/diff/11002/12002 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/11002/12002#newcode25 Line 25: int modifier = 1; On 2009/10/28 16:53:53, jcampan wrote: > int modified = forward ? -1 : 1; > would be simpler Done. I made it really mean "forward" now. Since |forward| in english means going forward :| so now it does it left to right. http://codereview.chromium.org/333010/diff/11002/12002#newcode34 Line 34: // Try to find the next available view that can be interacted. That view On 2009/10/28 16:53:53, jcampan wrote: > "interacted" -> interacted with Done. http://codereview.chromium.org/333010/diff/11002/12002#newcode44 Line 44: // Mo button is available in the specified direction, remains where it was. On 2009/10/28 16:53:53, jcampan wrote: > Comment wording Done. Done, misspelled no. http://codereview.chromium.org/333010/diff/11002/12002#newcode48 Line 48: bool AccessibleToolbarView::IsAccessibleViewTraversable(int current_view) { On 2009/10/28 16:53:53, jcampan wrote: > Why pass an index and not the view? It seems that it would be more convenient > for the subclasses to be passed a view. > Done. Your right, all the subclasses are using it directly instead. http://codereview.chromium.org/333010/diff/11002/12002#newcode200 Line 200: // Must have MSAA Focus inorder to handle ESC and TAB related key events. On 2009/10/28 16:53:53, jcampan wrote: > inorder -> in order Done. http://codereview.chromium.org/333010/diff/11002/12002#newcode209 Line 209: // |acc_focused_view_| doesn't need to be resetted here since it will be On 2009/10/28 16:53:53, jcampan wrote: > resetted -> reset Done. http://codereview.chromium.org/333010/diff/11002/12003 File chrome/browser/views/accessible_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/11002/12003#newcode18 Line 18: // tell the focus manager that this view is focusable while its not. The main On 2009/10/28 16:53:53, jcampan wrote: > Nit: its -> it's Done. http://codereview.chromium.org/333010/diff/11002/12003#newcode22 Line 22: class AccessibleToolbarView : public views::View { On 2009/10/28 16:53:53, jcampan wrote: > Insert blank line above. Done. http://codereview.chromium.org/333010/diff/11002/12003#newcode29 Line 29: // will navigate from right to left, and vice versa when false. -1 finds first On 2009/10/28 16:53:53, jcampan wrote: > Did you mean from left to right? Done. Why do I always get my directions wrong :( sad http://codereview.chromium.org/333010/diff/11002/12003#newcode38 Line 38: // Set the accessible MSAA focused view. On 2009/10/28 16:53:53, jcampan wrote: > Nit: set -> Sets. Done. http://codereview.chromium.org/333010/diff/11002/12003#newcode60 Line 60: // Child view currently having MSAA focus . On 2009/10/28 16:53:53, jcampan wrote: > MSAA is Windows specific. It should probably replaced with "accessibility". Done. http://codereview.chromium.org/333010/diff/11002/12004 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/333010/diff/11002/12004#newcode709 Line 709: if (GetAccFocusedChildView() == other_bookmarked_button_) { On 2009/10/28 16:53:53, jcampan wrote: > We could have other folders on the bookmark bar. Shouldn't this be generalized > to all MenuButtons? Yes, I will modify MenuButton to have a GetClassName (as Scott advised me on IM) so I can know its a MenuButton. http://codereview.chromium.org/333010/diff/11002/12007 File chrome/browser/views/detachable_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/11002/12007#newcode23 Line 23: DetachableToolbarView(BrowserView* parent) : AccessibleToolbarView(parent) {} On 2009/10/28 16:53:53, jcampan wrote: > explicit Done. http://codereview.chromium.org/333010/diff/11002/12010 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/333010/diff/11002/12010#newcode672 Line 672: toolbar_->RequestFocus(); On 2009/10/28 16:53:53, jcampan wrote: > It seems weird that we initialize the accessiblity traversal by requesting > focus. Should we have a StartAccessibilityTraversal method or something? > Also it seems |view_index| is not used. Done. For now, yes. http://codereview.chromium.org/333010/diff/11002/12011 File chrome/browser/views/frame/browser_view.h (right): http://codereview.chromium.org/333010/diff/11002/12011#newcode186 Line 186: // given navigation direction. |forward| when true, will navigate from right On 2009/10/28 16:53:53, jcampan wrote: > I find it confusing. Forward means RTL? I would have expected LTR? Done. I made it unconfusing, it now means RTL.
ping
http://codereview.chromium.org/333010/diff/16004/18003 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/333010/diff/16004/18003#newcode869 Line 869: AddChildView(other_bookmarked_button_); Why was this code moved? http://codereview.chromium.org/333010/diff/16004/18004 File chrome/browser/views/bookmark_bar_view.h (right): http://codereview.chromium.org/333010/diff/16004/18004#newcode76 Line 76: explicit BookmarkBarView(Profile* profile, Browser* browser); Remove explicit
http://codereview.chromium.org/333010/diff/16004/18003 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/333010/diff/16004/18003#newcode869 Line 869: AddChildView(other_bookmarked_button_); On 2009/11/02 21:20:23, jcampan wrote: > Why was this code moved? The reason why that was moved down there is because the order of traversing depends on the order of view, as explains to Scott in my earlier review. I explained why this is so in the accessible_toolbar_view class. The other bookmarks gets pushed till the end of the toolbar, where the overflow button always appears before other bookmarks, same thing goes for the sync button.
http://codereview.chromium.org/333010/diff/20014/22010 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/20014/22010#newcode7 Line 7: #include "chrome/browser/view_ids.h" view_ids should be either before browser_view, or after accessible_toolbar_view. http://codereview.chromium.org/333010/diff/20014/22010#newcode41 Line 41: // No button is available in the specified direction, remains where it was. 'remains where it was'? http://codereview.chromium.org/333010/diff/20014/22010#newcode42 Line 42: return view_index; Seems to me a return value of -1 would be much clearer here. http://codereview.chromium.org/333010/diff/20014/22010#newcode143 Line 143: break; Shouldn't this return rather than break? http://codereview.chromium.org/333010/diff/20014/22011 File chrome/browser/views/accessible_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/20014/22011#newcode12 Line 12: // toolbars within Chrome. It traverses child views in the order of adding them, It traverses -> Child views are traversed in the order they were added. http://codereview.chromium.org/333010/diff/20014/22011#newcode15 Line 15: // If a toolbar needs to be traversal then it needs to be focusable. But we do You can nuke this whole paragraph, it doesn't provide any information beyond the first paragraph. http://codereview.chromium.org/333010/diff/20014/22011#newcode28 Line 28: // view index, in the given navigation direction, |forward| when true means it the second , should be a . http://codereview.chromium.org/333010/diff/20014/22011#newcode30 Line 30: // |view_index| is -1, it finds the first accessible child. When |view_index| -> If |view_index| is -1 the first accessible child is returned. Also, you need to discuss the return value. http://codereview.chromium.org/333010/diff/20014/22011#newcode33 Line 33: // Invoked when you press left and right arrows while traversing in the view. Invoked from GetNextAccessibleViewIndex to determine if |view| can be traversed to. Default implementation returns true, override to return false for views you don't want reachable. http://codereview.chromium.org/333010/diff/20014/22011#newcode36 Line 36: virtual bool IsAccessibleViewTraversable(views::View* view); Should GetNext.. and IsAccessible... be protected? http://codereview.chromium.org/333010/diff/20014/22011#newcode39 Line 39: void set_acc_focused_view(views::View* acc_focused_view) { Does this really need to be public? http://codereview.chromium.org/333010/diff/20014/22011#newcode58 Line 58: void SetFocusToAccessibleView(); This method should be renamed. It doesn't actually give focus. http://codereview.chromium.org/333010/diff/20014/22012 File chrome/browser/views/bookmark_bar_view.cc (left): http://codereview.chromium.org/333010/diff/20014/22012#oldcode42 Line 42: #include "views/controls/button/menu_button.h" How come you nuked this include? http://codereview.chromium.org/333010/diff/20014/22012 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/333010/diff/20014/22012#newcode854 Line 854: sync_error_button_ = CreateSyncErrorButton(); You need a comment in here that order matters. http://codereview.chromium.org/333010/diff/20014/22015 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/333010/diff/20014/22015#newcode2064 Line 2064: DCHECK_EQ(new_height, 0); 0 should be first.
I have fixed all the nits, some minor changes I have done. I went back and introduced InitiateTraversal from Patch Set 1, that is needed because GetRootView()->GetFocusedView returns null if its not part of the focused view, that was one of the reasons why I introduced InitiateTraversal, cause it doesn't need to be called everytime it is focusing the toolbar. Please re-review. Comments inlined. http://codereview.chromium.org/333010/diff/20014/22010 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/20014/22010#newcode7 Line 7: #include "chrome/browser/view_ids.h" On 2009/11/03 16:50:15, sky wrote: > view_ids should be either before browser_view, or after accessible_toolbar_view. Done. http://codereview.chromium.org/333010/diff/20014/22010#newcode41 Line 41: // No button is available in the specified direction, remains where it was. On 2009/11/03 16:50:15, sky wrote: > 'remains where it was'? Done. http://codereview.chromium.org/333010/diff/20014/22010#newcode42 Line 42: return view_index; On 2009/11/03 16:50:15, sky wrote: > Seems to me a return value of -1 would be much clearer here. -1 is not valid because the beginning and the end of the traversal can be any index. It is perfectly valid for a traversal to start at index 3 because the first 3 child views are disabled/or hidden. So when we shift+tab, it should remain where it currently is because there is no button available traverse before that. http://codereview.chromium.org/333010/diff/20014/22010#newcode143 Line 143: break; On 2009/11/03 16:50:15, sky wrote: > Shouldn't this return rather than break? Your right, I rearranged those three lines to make it into one return exit. Its cleaner. http://codereview.chromium.org/333010/diff/20014/22011 File chrome/browser/views/accessible_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/20014/22011#newcode12 Line 12: // toolbars within Chrome. It traverses child views in the order of adding them, On 2009/11/03 16:50:15, sky wrote: > It traverses -> Child views are traversed in the order they were added. Done. http://codereview.chromium.org/333010/diff/20014/22011#newcode15 Line 15: // If a toolbar needs to be traversal then it needs to be focusable. But we do On 2009/11/03 16:50:15, sky wrote: > You can nuke this whole paragraph, it doesn't provide any information beyond the > first paragraph. Done. http://codereview.chromium.org/333010/diff/20014/22011#newcode28 Line 28: // view index, in the given navigation direction, |forward| when true means it On 2009/11/03 16:50:15, sky wrote: > the second , should be a . Done. http://codereview.chromium.org/333010/diff/20014/22011#newcode30 Line 30: // |view_index| is -1, it finds the first accessible child. On 2009/11/03 16:50:15, sky wrote: > When |view_index| -> If |view_index| is -1 the first accessible child is > returned. > > Also, you need to discuss the return value. Done. http://codereview.chromium.org/333010/diff/20014/22011#newcode33 Line 33: // Invoked when you press left and right arrows while traversing in the view. On 2009/11/03 16:50:15, sky wrote: > Invoked from GetNextAccessibleViewIndex to determine if |view| can be traversed > to. Default implementation returns true, override to return false for views you > don't want reachable. Done. Your comments are professional, one of my future goals :/ http://codereview.chromium.org/333010/diff/20014/22011#newcode36 Line 36: virtual bool IsAccessibleViewTraversable(views::View* view); On 2009/11/03 16:50:15, sky wrote: > Should GetNext.. and IsAccessible... be protected? Yes they should, I changed them accordingly. http://codereview.chromium.org/333010/diff/20014/22011#newcode39 Line 39: void set_acc_focused_view(views::View* acc_focused_view) { On 2009/11/03 16:50:15, sky wrote: > Does this really need to be public? Done. I nuked it, since we don't even need it. It is legacy. http://codereview.chromium.org/333010/diff/20014/22011#newcode58 Line 58: void SetFocusToAccessibleView(); On 2009/11/03 16:50:15, sky wrote: > This method should be renamed. It doesn't actually give focus. Done. http://codereview.chromium.org/333010/diff/20014/22012 File chrome/browser/views/bookmark_bar_view.cc (left): http://codereview.chromium.org/333010/diff/20014/22012#oldcode42 Line 42: #include "views/controls/button/menu_button.h" On 2009/11/03 16:50:15, sky wrote: > How come you nuked this include? Because it is duplicated. Look at line 40. Same include. http://codereview.chromium.org/333010/diff/20014/22012 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/333010/diff/20014/22012#newcode854 Line 854: sync_error_button_ = CreateSyncErrorButton(); On 2009/11/03 16:50:15, sky wrote: > You need a comment in here that order matters. Done. http://codereview.chromium.org/333010/diff/20014/22015 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/333010/diff/20014/22015#newcode2064 Line 2064: DCHECK_EQ(new_height, 0); On 2009/11/03 16:50:15, sky wrote: > 0 should be first. Done.
Almost there. You need to deal with selected_focused_view_ being removed. You can deal with this by overriding ViewHierarchyChanged. http://codereview.chromium.org/333010/diff/25001/25002 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/25001/25002#newcode24 Line 24: void AccessibleToolbarView::InitiateTraversal() { I don't understand why this isn't requestFocus. You mentioned to me it was because "GetFocusView() will return NULL if the focused view does not belong to that RootView", but yet you're still calling GetRootView here. What am I missing? http://codereview.chromium.org/333010/diff/25001/25002#newcode58 Line 58: return view_index; Any reason we don't want this to return -1? It means you end up with code like: int new_view = GetNextAccessibleViewIndex(...); if (next_view == -1 || last_view == new_view) return; Instead of just: new_view = GetNextAccessibleViewIndex(...); if (next_view == -1) return; http://codereview.chromium.org/333010/diff/25001/25004 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/333010/diff/25001/25004#newcode854 Line 854: // Order matters here. Child views are traversed in the order they were added. Child views are traversed in the order they are added. Make sure the order they are added matches the visual order.
Sorry guys, I came back from my conference (was away from the computer for ~6 days). Thanks for your comments Scott, I am trying to understand what you meant regarding selected_focused_view_. I overrideed ViewHierarchyChanged, but I don't know if that is what you want. I basically just checked if the view is being removed, if it is, I traverse to the next toolbar. Is that correct? Thanks for the reviews! I know you guys are busy with milestone 4 and chromeos, but I appreciate all this. http://codereview.chromium.org/333010/diff/25001/25002 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/25001/25002#newcode24 Line 24: void AccessibleToolbarView::InitiateTraversal() { On 2009/11/06 21:46:02, sky wrote: > I don't understand why this isn't requestFocus. You mentioned to me it was > because "GetFocusView() will return NULL if the focused view does not belong to > that RootView", but yet you're still calling GetRootView here. What am I > missing? By separating them (RequestFocus, and InitiateTraversal) it wasn't crashing anymore. But I merged them together now, and no more crashes. But let me explain to you why I want them to be separate. One of the reasons I want to separate is because one of my todo's in BrowserView. When we traverse between toolbars, it currently just traverses between Toolbar and BookmarksBar, but after this CL, I am going to work on making it work on all Chrome toolbars. I might be planning to expand InitiateTraversal and add more parameters to make it work. I am afraid if I merge them together it will cause crashes again :( http://codereview.chromium.org/333010/diff/25001/25002#newcode58 Line 58: return view_index; On 2009/11/06 21:46:02, sky wrote: > Any reason we don't want this to return -1? > It means you end up with code like: > int new_view = GetNextAccessibleViewIndex(...); > if (next_view == -1 || last_view == new_view) > return; > > Instead of just: > new_view = GetNextAccessibleViewIndex(...); > if (next_view == -1) > return; Done. I double checked and -1 works fine here, thanks. http://codereview.chromium.org/333010/diff/25001/25004 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/333010/diff/25001/25004#newcode854 Line 854: // Order matters here. Child views are traversed in the order they were added. On 2009/11/06 21:46:02, sky wrote: > Child views are traversed in the order they are added. Make sure the order they > are added matches the visual order. Done.
Please review patchset 8. Some comments. I needed to save the last focused view once I first initiate the first traversal (Alt+Shift+T), and pass its view storage id to AccessibleToolbarTraversal because when we traverse toolbars, we need to take save the last focused view somewhere, and ViewStorage is the best place for that since its a Singleton. Another comment is that I refactored one method since I can reuse it. AccessibleToolbarView::SetFocusToLastFocusedView to handle the quiting traversal. Jonas and I decided to rerun traversal once the user activates Alt+Shift+T. Please take a look and let me know any comments. So far, it doesn't crash, and it runs as expected.
http://codereview.chromium.org/333010/diff/37003/37004 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/37003/37004#newcode229 chrome/browser/views/accessible_toolbar_view.cc:229: if (selected_focused_view_ && !is_add) You should only do this if child is selected_focus_view_, and don't you need to set selected_focus_view_ to NULL here? http://codereview.chromium.org/333010/diff/37003/37005 File chrome/browser/views/accessible_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/37003/37005#newcode43 chrome/browser/views/accessible_toolbar_view.h:43: int GetNextAccessibleViewIndex(int view_index, bool forward); You never answered me as to why this can't return a View instead of an int? It'll make for easier to write code.
Thanks for the reviews, I followed your comments. Hopefully this is good! http://codereview.chromium.org/333010/diff/37003/37004 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/37003/37004#newcode229 chrome/browser/views/accessible_toolbar_view.cc:229: if (selected_focused_view_ && !is_add) On 2009/12/02 22:54:19, sky wrote: > You should only do this if child is selected_focus_view_, and don't you need to > set selected_focus_view_ to NULL here? Done. http://codereview.chromium.org/333010/diff/37003/37005 File chrome/browser/views/accessible_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/37003/37005#newcode43 chrome/browser/views/accessible_toolbar_view.h:43: int GetNextAccessibleViewIndex(int view_index, bool forward); On 2009/12/02 22:54:19, sky wrote: > You never answered me as to why this can't return a View instead of an int? > It'll make for easier to write code. Done. Your right thanks.
Almost there. http://codereview.chromium.org/333010/diff/40014/39003 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/40014/39003#newcode112 chrome/browser/views/accessible_toolbar_view.cc:112: // Paranoia check, button should be initialized upon toolbar gaining focus. I believe this can happen if there are no child views when you gained focus or the selected view is removed. I think you should continue and update line 156 to deal with null. http://codereview.chromium.org/333010/diff/40014/39004 File chrome/browser/views/accessible_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/40014/39004#newcode42 chrome/browser/views/accessible_toolbar_view.h:42: // returned. Mention the return value.
Thanks Scott, I renamed some of the ones to HasFocus (as we discussed in IM) that makes it make "more" sense and works. I added a check to the key's default switch to see if its null. If you don't like the style, let me know :) http://codereview.chromium.org/333010/diff/40014/39003 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/40014/39003#newcode112 chrome/browser/views/accessible_toolbar_view.cc:112: // Paranoia check, button should be initialized upon toolbar gaining focus. On 2009/12/03 00:11:00, sky wrote: > I believe this can happen if there are no child views when you gained focus or > the selected view is removed. I think you should continue and update line 156 to > deal with null. Done. As we discussed, I used HasFocus in some places which is more readable and added a check in line #148 http://codereview.chromium.org/333010/diff/40014/39004 File chrome/browser/views/accessible_toolbar_view.h (right): http://codereview.chromium.org/333010/diff/40014/39004#newcode42 chrome/browser/views/accessible_toolbar_view.h:42: // returned. On 2009/12/03 00:11:00, sky wrote: > Mention the return value. Done. Is this good enough? I mentioned it at the beginning.
http://codereview.chromium.org/333010/diff/41004/41005 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/41004/41005#newcode112 chrome/browser/views/accessible_toolbar_view.cc:112: // Paranoia check, button should be initialized upon toolbar gaining focus. nuke this comment. http://codereview.chromium.org/333010/diff/41004/41005#newcode128 chrome/browser/views/accessible_toolbar_view.cc:128: if (selected_focused_view_->GetClassName() == need null check here. http://codereview.chromium.org/333010/diff/41004/41005#newcode170 chrome/browser/views/accessible_toolbar_view.cc:170: // Paranoia check, button should be initialized upon toolbar gaining focus. nuke this comment. http://codereview.chromium.org/333010/diff/41004/41005#newcode175 chrome/browser/views/accessible_toolbar_view.cc:175: return selected_focused_view_->OnKeyReleased(e); need null check here.
Added the extra checks. http://codereview.chromium.org/333010/diff/41004/41005 File chrome/browser/views/accessible_toolbar_view.cc (right): http://codereview.chromium.org/333010/diff/41004/41005#newcode112 chrome/browser/views/accessible_toolbar_view.cc:112: // Paranoia check, button should be initialized upon toolbar gaining focus. On 2009/12/03 17:20:24, sky wrote: > nuke this comment. Done. http://codereview.chromium.org/333010/diff/41004/41005#newcode128 chrome/browser/views/accessible_toolbar_view.cc:128: if (selected_focused_view_->GetClassName() == On 2009/12/03 17:20:24, sky wrote: > need null check here. Done. http://codereview.chromium.org/333010/diff/41004/41005#newcode170 chrome/browser/views/accessible_toolbar_view.cc:170: // Paranoia check, button should be initialized upon toolbar gaining focus. On 2009/12/03 17:20:24, sky wrote: > nuke this comment. Done. http://codereview.chromium.org/333010/diff/41004/41005#newcode175 chrome/browser/views/accessible_toolbar_view.cc:175: return selected_focused_view_->OnKeyReleased(e); On 2009/12/03 17:20:24, sky wrote: > need null check here. Done.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
