Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
380943002:
Added battery level and time to the status tray's accessible name. (Closed)
|
Created:
6 years, 5 months ago by evy Modified:
6 years, 5 months ago CC:
chromium-reviews, sadrul, dtseng+watch_chromium.org, derat+watch_chromium.org, aboxhall+watch_chromium.org, tfarina, yuzo+watch_chromium.org, dmazzoni+watch_chromium.org, oshima+watch_chromium.org, kalyank, stevenjb+watch_chromium.org, plundblad+watch_chromium.org, ben+ash_chromium.org, lisayin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdded battery level and time to the status tray's accessible name.
Appended already written accessible strings for time and battery to the status tray's current name.
Ensured the dispatched accessibility event for any events coming from the status tray reads the status tray name and not the names of the buttons inside the status tray bar.
BUG=392666
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284818
Patch Set 1 #
Total comments: 12
Patch Set 2 : no longer adding strings, time now says 'current time' #
Total comments: 10
Patch Set 3 : #
Total comments: 12
Patch Set 4 : fixed nits #Patch Set 5 : view storage #
Total comments: 12
Patch Set 6 : FindFirstAccessibleAncestor function #
Total comments: 1
Patch Set 7 : #Patch Set 8 : battery only read on chromeos builds #Patch Set 9 : removed old view before storing a new one #Messages
Total messages: 38 (0 generated)
https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.... ash/system/tray/system_tray.cc:595: return base::TimeFormatTimeOfDayWithHourClockType( Just to make sure, can you try this with the setting for 24 hour time instead of 12 hour time. https://codereview.chromium.org/380943002/diff/1/chrome/browser/ui/views/acce... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/1/chrome/browser/ui/views/acce... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:183: most_recent_view_ && view must be non-null here, so this check is redundant AFAICT.
https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.h File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.... ash/system/tray/system_tray.h:124: // Returns a string with the current time for accessiblity on the status nit: accessibility https://codereview.chromium.org/380943002/diff/1/chrome/browser/ui/views/acce... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/1/chrome/browser/ui/views/acce... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:29: : most_recent_profile_(NULL), most_recent_view_(NULL) { nit: one per line if they don't all fit on the same line as the method signature https://codereview.chromium.org/380943002/diff/1/chrome/browser/ui/views/acce... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:183: most_recent_view_ && nit: fix indenting also, view seems to be non-NULL based on the previous code, so this line doesn't seem like it's necessary
https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.... ash/system/tray/system_tray.cc:631: return name + base::ASCIIToUTF16(". ") + This is borderline, but this is usually the best practice for string combining, rather than concatenating. l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_ACCESSIBLE_DESCRIPTION, PowerStatus::Get()->GetShortAccessibleNameString(), GetAccessibleTimeString(base::Time::Now()) where the string is something like: "Status tray, $1, current time $2"
+jshin for any advice on i18n - is it ever okay to concatenate strings, or should you always use a message?
https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.... ash/system/tray/system_tray.cc:595: return base::TimeFormatTimeOfDayWithHourClockType( On 2014/07/09 23:08:11, Peter Lundblad wrote: > Just to make sure, can you try this with the setting for 24 hour time instead of > 12 hour time. 24 hour time is read very strange (oh nine. one one.) But this is a thing that already happened in the current status tray time reading, and Dominic said it's probably out of our project scope. However, nothing breaks - so that's good! I think I'll have it read the word time before it reads the time, so that it's clear that "oh nine. one one." is a time. https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.... ash/system/tray/system_tray.cc:631: return name + base::ASCIIToUTF16(". ") + On 2014/07/10 16:23:16, dmazzoni wrote: > This is borderline, but this is usually the best practice for string combining, > rather than concatenating. > > l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_ACCESSIBLE_DESCRIPTION, > PowerStatus::Get()->GetShortAccessibleNameString(), > GetAccessibleTimeString(base::Time::Now()) > > where the string is something like: > > "Status tray, $1, current time $2" Done. https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.h File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/380943002/diff/1/ash/system/tray/system_tray.... ash/system/tray/system_tray.h:124: // Returns a string with the current time for accessiblity on the status On 2014/07/09 23:10:24, Daniel Erat wrote: > nit: accessibility Done. https://codereview.chromium.org/380943002/diff/1/chrome/browser/ui/views/acce... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/1/chrome/browser/ui/views/acce... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:29: : most_recent_profile_(NULL), most_recent_view_(NULL) { On 2014/07/09 23:10:24, Daniel Erat wrote: > nit: one per line if they don't all fit on the same line as the method signature Done. https://codereview.chromium.org/380943002/diff/1/chrome/browser/ui/views/acce... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:183: most_recent_view_ && On 2014/07/09 23:10:24, Daniel Erat wrote: > nit: fix indenting > > also, view seems to be non-NULL based on the previous code, so this line doesn't > seem like it's necessary Done. https://codereview.chromium.org/380943002/diff/1/chrome/browser/ui/views/acce... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:183: most_recent_view_ && On 2014/07/09 23:08:11, Peter Lundblad wrote: > view must be non-null here, so this check is redundant AFAICT. Done.
https://codereview.chromium.org/380943002/diff/40001/ash/system/chromeos/powe... File ash/system/chromeos/power/power_status.cc (right): https://codereview.chromium.org/380943002/diff/40001/ash/system/chromeos/powe... ash/system/chromeos/power/power_status.cc:280: base::string16 PowerStatus::GetShortAccessibleNameString() const { Can you call this function from GetAccessibleNameString, above, and avoid duplicating code? https://codereview.chromium.org/380943002/diff/40001/ash/system/tray/system_t... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/380943002/diff/40001/ash/system/tray/system_t... ash/system/tray/system_tray.h:126: base::string16 GetAccessibleTimeString(const base::Time& now) const; This should be private. https://codereview.chromium.org/380943002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:168: // focusable views inside other focusable views (e.g. hover for date text Actually this handles *any* views inside focusable views. https://codereview.chromium.org/380943002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:172: while (view->parent()) { It seems a little confusing to have "view" walk up its parents, and then reset it to highest_focusable_view at the end. I would create a temporary View for the loop, and just update |view| if you find an ancestor that's focusable. Alternatively, write a helper function FocusableAncestor, and update |view| to that if it returns something other than NULL
https://codereview.chromium.org/380943002/diff/40001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/380943002/diff/40001/ash/ash_strings.grd#newc... ash/ash_strings.grd:208: Status tray, current time <ph name="time">$1<ex>9:50</ex></ph>, <ph name="battery">$2<ex>Battery is full.</ex></ph> 'current' seems unnecessary and takes up precious space on a braille display, can we just say 'time ...'
https://codereview.chromium.org/380943002/diff/40001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/380943002/diff/40001/ash/ash_strings.grd#newc... ash/ash_strings.grd:208: Status tray, current time <ph name="time">$1<ex>9:50</ex></ph>, <ph name="battery">$2<ex>Battery is full.</ex></ph> On 2014/07/10 17:33:34, Peter Lundblad wrote: > 'current' seems unnecessary and takes up precious space on a braille display, > can we just say 'time ...' Done. https://codereview.chromium.org/380943002/diff/40001/ash/system/chromeos/powe... File ash/system/chromeos/power/power_status.cc (right): https://codereview.chromium.org/380943002/diff/40001/ash/system/chromeos/powe... ash/system/chromeos/power/power_status.cc:280: base::string16 PowerStatus::GetShortAccessibleNameString() const { On 2014/07/10 17:28:20, dmazzoni wrote: > Can you call this function from GetAccessibleNameString, above, and avoid > duplicating code? Done. https://codereview.chromium.org/380943002/diff/40001/ash/system/tray/system_t... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/380943002/diff/40001/ash/system/tray/system_t... ash/system/tray/system_tray.h:126: base::string16 GetAccessibleTimeString(const base::Time& now) const; On 2014/07/10 17:28:21, dmazzoni wrote: > This should be private. Done. https://codereview.chromium.org/380943002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:168: // focusable views inside other focusable views (e.g. hover for date text On 2014/07/10 17:28:21, dmazzoni wrote: > Actually this handles *any* views inside focusable views. Done. https://codereview.chromium.org/380943002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:172: while (view->parent()) { On 2014/07/10 17:28:21, dmazzoni wrote: > It seems a little confusing to have "view" walk up its parents, and then reset > it to highest_focusable_view at the end. > > I would create a temporary View for the loop, and just update |view| if you find > an ancestor that's focusable. > > Alternatively, write a helper function FocusableAncestor, and update |view| to > that if it returns something other than NULL > Done.
lgtm
lgtm https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:175: if (temp_view->IsFocusable()) { Nit: in C++, braces aren't needed when the "if" condition and the statement each fit on one line.
+sky for OWNERS review
@sky: please have a look at accessibility_event_router_views.*
https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:167: // The highest focusable view is processed to handle cases where there are It's not clear to me why you want the highest focusable view? If you hover over |view| isn't |view| the interesting view and not some ancestor? Also, if you are going to look for a focusable ancestor shouldn't you stop at the first one? I suspect you want the first ancestor (including view) that would handle the mouse event, but I'm not sure there. How is this code called? https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:169: // status bar) and the status bar information should be read instead nit: you've got an extra space at the beginning. https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:185: most_recent_view_ = view; I don't see this ever set to NULL. What if most_recent_view_ is deleted?
https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:167: // The highest focusable view is processed to handle cases where there are On 2014/07/11 15:44:02, sky wrote: > It's not clear to me why you want the highest focusable view? If you hover over > |view| isn't |view| the interesting view and not some ancestor? Also, if you are > going to look for a focusable ancestor shouldn't you stop at the first one? > > I suspect you want the first ancestor (including view) that would handle the > mouse event, but I'm not sure there. How is this code called? If a view is focusable, it's usually atomic wrt accessibility. The example this is fixing is the status tray button. The button itself is focusable and has proper accessibility attributes, and if you focus it with the keyboard it does the right thing. It also has several child views - the battery, time, some icons - but when hovering over these views it doesn't make sense to announce them separately, because they're all just part of the same button. https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:185: most_recent_view_ = view; On 2014/07/11 15:44:02, sky wrote: > I don't see this ever set to NULL. What if most_recent_view_ is deleted? It should be harmless because all we ever do is test whether or not most_recent_view_ is equal to the current view. Would you prefer ViewStorage here instead, or is this fine, maybe with just a comment indicating that most_recent_view_ may be invalid and we should only use it for equality comparison.
https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:169: // status bar) and the status bar information should be read instead On 2014/07/11 15:44:02, sky wrote: > nit: you've got an extra space at the beginning. Done. https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:175: if (temp_view->IsFocusable()) { On 2014/07/10 23:25:58, dmazzoni wrote: > Nit: in C++, braces aren't needed when the "if" condition and the statement each > fit on one line. Done.
https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:167: // The highest focusable view is processed to handle cases where there are On 2014/07/11 16:11:38, dmazzoni wrote: > On 2014/07/11 15:44:02, sky wrote: > > It's not clear to me why you want the highest focusable view? If you hover > over > > |view| isn't |view| the interesting view and not some ancestor? Also, if you > are > > going to look for a focusable ancestor shouldn't you stop at the first one? > > > > I suspect you want the first ancestor (including view) that would handle the > > mouse event, but I'm not sure there. How is this code called? > > If a view is focusable, it's usually atomic wrt accessibility. > > The example this is fixing is the status tray button. The button itself is > focusable and has proper accessibility attributes, and if you focus it with the > keyboard it does the right thing. > > It also has several child views - the battery, time, some icons - but when > hovering over these views it doesn't make sense to announce them separately, > because they're all just part of the same button. What mechanism does the status tray button use to ensure only it shows active? Hit targetting? Seems like this code should do the same. https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:185: most_recent_view_ = view; On 2014/07/11 16:11:38, dmazzoni wrote: > On 2014/07/11 15:44:02, sky wrote: > > I don't see this ever set to NULL. What if most_recent_view_ is deleted? > > It should be harmless because all we ever do is test whether or not > most_recent_view_ is equal to the current view. Two concerns: there is nothing stopping the pointer from getting recycled, and since the member is a view there is nothing stopping code added later on attempting to use the member as a View and not realizing it may be deleted. > Would you prefer ViewStorage here instead, or is this fine, maybe with just a > comment indicating that most_recent_view_ may be invalid and we should only use > it for equality comparison. I prefer view storage.
https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:167: // The highest focusable view is processed to handle cases where there are On 2014/07/11 19:06:08, sky wrote: > On 2014/07/11 16:11:38, dmazzoni wrote: > > On 2014/07/11 15:44:02, sky wrote: > > > It's not clear to me why you want the highest focusable view? If you hover > > over > > > |view| isn't |view| the interesting view and not some ancestor? Also, if you > > are > > > going to look for a focusable ancestor shouldn't you stop at the first one? > > > > > > I suspect you want the first ancestor (including view) that would handle the > > > mouse event, but I'm not sure there. How is this code called? > > > > If a view is focusable, it's usually atomic wrt accessibility. > > > > The example this is fixing is the status tray button. The button itself is > > focusable and has proper accessibility attributes, and if you focus it with > the > > keyboard it does the right thing. > > > > It also has several child views - the battery, time, some icons - but when > > hovering over these views it doesn't make sense to announce them separately, > > because they're all just part of the same button. > > What mechanism does the status tray button use to ensure only it shows active? > Hit targetting? Seems like this code should do the same. Aha. If I go to the next highest accessibility focusable view, it works. Does this implementation seem okay to you? https://codereview.chromium.org/380943002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:185: most_recent_view_ = view; On 2014/07/11 19:06:08, sky wrote: > On 2014/07/11 16:11:38, dmazzoni wrote: > > On 2014/07/11 15:44:02, sky wrote: > > > I don't see this ever set to NULL. What if most_recent_view_ is deleted? > > > > It should be harmless because all we ever do is test whether or not > > most_recent_view_ is equal to the current view. > > Two concerns: there is nothing stopping the pointer from getting recycled, and > since the member is a view there is nothing stopping code added later on > attempting to use the member as a View and not realizing it may be deleted. > > > Would you prefer ViewStorage here instead, or is this fine, maybe with just a > > comment indicating that most_recent_view_ may be invalid and we should only > use > > it for equality comparison. > > I prefer view storage. Done.
https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:174: view = temp_view; Don't you want to break here? https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.h (right): https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.h:170: int most_recent_view_id_; const
https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:174: view = temp_view; On 2014/07/14 15:57:19, sky wrote: > Don't you want to break here? Well, I have to check it in the while condition in case the original view is already focusable, and the while condition should cause it to break on the next iteration. Do you think it would be more clear to have the explicit break? If so, I can add it. https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.h (right): https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.h:170: int most_recent_view_id_; On 2014/07/14 15:57:19, sky wrote: > const Done.
https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:174: view = temp_view; On 2014/07/14 16:03:47, evy wrote: > On 2014/07/14 15:57:19, sky wrote: > > Don't you want to break here? > > Well, I have to check it in the while condition in case the original view is > already focusable, and the while condition should cause it to break on the next > iteration. > > Do you think it would be more clear to have the explicit break? If so, I can add > it. It may not be an actual problem in this case since IsAccessibilityFocusable is a quick function call, but I'd say it's usually better style to write a loop that doesn't test the loop condition twice. https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:181: views::ViewStorage::GetInstance()->RetrieveView(most_recent_view_id_) == Optional: add "using views::ViewStorage;" at the top of the file and replace views::ViewStorage with just ViewStorage, will make this slightly more readable - especially since we're using it in two different places within this file. https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:183: return; Nit: { } around return because conditional is multi-line
https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:183: return; On 2014/07/14 17:46:00, dmazzoni wrote: > Nit: { } around return because conditional is multi-line i don't feel strongly about this one way or another, but the rule that i think i've typically heard (and then repeated myself) is "curly brackets if the statement _body_ is multi-line, but not if only the condition is"
https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:174: view = temp_view; On 2014/07/14 16:03:47, evy wrote: > On 2014/07/14 15:57:19, sky wrote: > > Don't you want to break here? > > Well, I have to check it in the while condition in case the original view is > already focusable, and the while condition should cause it to break on the next > iteration. > > Do you think it would be more clear to have the explicit break? If so, I can add > it. Are you saying you want: while (temp_view && !temp_view->IsAF()) temp_view = temp_view->parent(); if (temp_view) view = temp_view; For clarity (and ease of testing) you could move to a standalone function so that this code becomes something like: view = FindFirstAccessibleAncestor(view); ?
https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc (right): https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:174: view = temp_view; On 2014/07/14 21:11:14, sky wrote: > On 2014/07/14 16:03:47, evy wrote: > > On 2014/07/14 15:57:19, sky wrote: > > > Don't you want to break here? > > > > Well, I have to check it in the while condition in case the original view is > > already focusable, and the while condition should cause it to break on the > next > > iteration. > > > > Do you think it would be more clear to have the explicit break? If so, I can > add > > it. > > Are you saying you want: > > while (temp_view && !temp_view->IsAF()) > temp_view = temp_view->parent(); > if (temp_view) > view = temp_view; > > For clarity (and ease of testing) you could move to a standalone function so > that this code becomes something like: > view = FindFirstAccessibleAncestor(view); > > ? Yeah, sorry this function is way more complicated than it needs to be because I originally wrote it for the highest accessible view and it's still structured for that. It's only a three line function, but I think it's nice to have it in its own function still. https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:181: views::ViewStorage::GetInstance()->RetrieveView(most_recent_view_id_) == On 2014/07/14 17:46:00, dmazzoni wrote: > Optional: add "using views::ViewStorage;" at the top of the file and replace > views::ViewStorage with just ViewStorage, will make this slightly more readable > - especially since we're using it in two different places within this file. Done. https://codereview.chromium.org/380943002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:183: return; On 2014/07/14 18:04:14, Daniel Erat wrote: > On 2014/07/14 17:46:00, dmazzoni wrote: > > Nit: { } around return because conditional is multi-line > > i don't feel strongly about this one way or another, but the rule that i think > i've typically heard (and then repeated myself) is "curly brackets if the > statement _body_ is multi-line, but not if only the condition is" I've been taught that it's nice to have curly braces just in case you add something later and forget to add the braces - so I think I'll add them.
LGTM https://codereview.chromium.org/380943002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/accessibility/accessibility_event_router_views.h (right): https://codereview.chromium.org/380943002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/accessibility/accessibility_event_router_views.h:161: // If a view is not focusable for accessibility, find the closest This comment is mildly confusing. I think you mean: 'Returns the first ancestor of |view| (including |view)| that is accessible.'
The CQ bit was checked by evy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evy@chromium.org/380943002/140001
The CQ bit was unchecked by evy@chromium.org
The CQ bit was checked by evy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evy@chromium.org/380943002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by evy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evy@chromium.org/380943002/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 284818 |
