Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
464703002:
Fixed a11y tab-handling for language settings (Closed)
|
Created:
6 years, 4 months ago by hcarmona Modified:
6 years, 4 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, nona+watch_chromium.org, yukishiino+watch_chromium.org, arv+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFixed a11y tab-handling for language settings
The fix is to remove the tab index from the X button in the list so that when a user presses <TAB>, the next control has focus instead of the next item in the list.
Tests that were added:
- Verify that appropriate languages are assigned to page. This is to make sure that the list of languages remains deterministic. This also has the effect of failing the test if we do not navigate to the language options.
- Verify that when the correct element has focus after the <TAB> key is pressed when the list had focus. This will verify that the bug is fixed correctly.
BUG=399352
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290651
Patch Set 1 #
Total comments: 20
Patch Set 2 : Make changes based on feedback #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : Upload for trybot #Patch Set 6 : Change to fix trybox failure #
Total comments: 8
Patch Set 7 : Make changes based on feedback #
Total comments: 9
Patch Set 8 : Changes based on feedback from groby@ #
Total comments: 10
Patch Set 9 : Changes based on feedback from dbeam@ #
Messages
Total messages: 19 (0 generated)
A few drive-by comments, and thank you for adding a test case! https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/language_options_interactive_uitest.cc (right): https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:38: const GURL& url = chrome::GetSettingsUrl(chrome::kLanguageOptionsSubPage); Make this non-ref. GetSettingsUrl returns an object, not a reference. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:57: bool GetActiveFrame(content::RenderFrameHost* &active_frame) { No outparams via ref, sorry :( - Here and elsewhere. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:59: if (GetActiveWebContents(web_contents)) { If this "should never happen", it's OK to just get the web_contents and not check, if the failure case returns NULL. (Having a crashing test will still tell us there's a bug) https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:82: if (!GetNativeWindow(native_window)) NULL check not needed if this should always be not NULL. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:93: bool GetNativeWindow(gfx::NativeWindow &native_window) { Chrome's code standard doesn't allow out parameters via references. You want either a pointer as out param, or you could just return by value - it's OK to check gfx::NativeWindow against NULL. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:113: // Is this test necessary? All other tests will fail if this one fails. If this is a precondition, I'd handle this in the test's SetUp and just ASSERT the conditions - side effect, all further tests will also already be navigated to the language page. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:114: // How to handle change in title format? (that would break this test) Don't worry about that - it's up to the person changing the title format. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:162: // Cannot translate a XKeyEvent to a GdkEvent. Might want to ping yukishiino@ about this. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:177: // make sure that the next element is the button that is next in the tab order Can you instead just test that it's moved on to a new element? Otherwise this'll break as soon as somebody changes tab order. https://codereview.chromium.org/464703002/diff/1/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/464703002/diff/1/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:57: '../chrome/browser/ui/webui/options/language_options_interactive_uitest.cc', The list is alphabetized, so you want to move this up.
I made fixes to the code based on your feedback. Test is simpler, which is better. Also, found another way to send the key presses because when I tried this on my other machine, it wasn't working there. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/language_options_interactive_uitest.cc (right): https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:38: const GURL& url = chrome::GetSettingsUrl(chrome::kLanguageOptionsSubPage); On 2014/08/12 23:57:27, groby wrote: > Make this non-ref. GetSettingsUrl returns an object, not a reference. Done. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:57: bool GetActiveFrame(content::RenderFrameHost* &active_frame) { On 2014/08/12 23:57:27, groby wrote: > No outparams via ref, sorry :( - Here and elsewhere. Done. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:59: if (GetActiveWebContents(web_contents)) { On 2014/08/12 23:57:27, groby wrote: > If this "should never happen", it's OK to just get the web_contents and not > check, if the failure case returns NULL. (Having a crashing test will still tell > us there's a bug) Done. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:82: if (!GetNativeWindow(native_window)) On 2014/08/12 23:57:27, groby wrote: > NULL check not needed if this should always be not NULL. Done. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:93: bool GetNativeWindow(gfx::NativeWindow &native_window) { On 2014/08/12 23:57:27, groby wrote: > Chrome's code standard doesn't allow out parameters via references. You want > either a pointer as out param, or you could just return by value - it's OK to > check gfx::NativeWindow against NULL. Done. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:113: // Is this test necessary? All other tests will fail if this one fails. On 2014/08/12 23:57:28, groby wrote: > If this is a precondition, I'd handle this in the test's SetUp and just ASSERT > the conditions - side effect, all further tests will also already be navigated > to the language page. Done. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:114: // How to handle change in title format? (that would break this test) On 2014/08/12 23:57:27, groby wrote: > Don't worry about that - it's up to the person changing the title format. Acknowledged. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:162: // Cannot translate a XKeyEvent to a GdkEvent. Found another way to send keyboard events that gets rid of these errors and also works on my mac. On 2014/08/12 23:57:27, groby wrote: > Might want to ping yukishiino@ about this. https://codereview.chromium.org/464703002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:177: // make sure that the next element is the button that is next in the tab order Kept this expectation the same because we want to make sure we're not tabbing to another element inside the list with a different id. On 2014/08/12 23:57:27, groby wrote: > Can you instead just test that it's moved on to a new element? Otherwise this'll > break as soon as somebody changes tab order. https://codereview.chromium.org/464703002/diff/1/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/464703002/diff/1/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:57: '../chrome/browser/ui/webui/options/language_options_interactive_uitest.cc', On 2014/08/12 23:57:28, groby wrote: > The list is alphabetized, so you want to move this up. Done.
Please make the title of this issue more descriptive than 'a UI test'. Be sure to change the description along with the subject. https://codereview.chromium.org/464703002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/language_options_interactive_uitest.cc (right): https://codereview.chromium.org/464703002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:55: &element_id)) nit: Unless we want to log a warning here, the if() should be unnecessary, we can just return element_id (which should be unset if it fails) https://codereview.chromium.org/464703002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:72: false, false, false, false); align args https://codereview.chromium.org/464703002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:101: "domAutomationController.send(count);", We should define this test string separately. Embedding it in the function call is confusing to read. https://codereview.chromium.org/464703002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:109: "domAutomationController.send(document.activeElement.textContent);", ditto
Updated description of issue, and applied changes from feedback. https://codereview.chromium.org/464703002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/language_options_interactive_uitest.cc (right): https://codereview.chromium.org/464703002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:55: &element_id)) On 2014/08/18 16:05:06, stevenjb wrote: > nit: Unless we want to log a warning here, the if() should be unnecessary, we > can just return element_id (which should be unset if it fails) Done. https://codereview.chromium.org/464703002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:72: false, false, false, false); On 2014/08/18 16:05:06, stevenjb wrote: > align args Done. https://codereview.chromium.org/464703002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:101: "domAutomationController.send(count);", On 2014/08/18 16:05:06, stevenjb wrote: > We should define this test string separately. Embedding it in the function call > is confusing to read. Done. https://codereview.chromium.org/464703002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:109: "domAutomationController.send(document.activeElement.textContent);", On 2014/08/18 16:05:05, stevenjb wrote: > ditto Done.
Sorry, a few more drive-by comments https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/language_options_interactive_uitest.cc (right): https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:25: // We need to emulate button pushing because we are testing accessibility, Nit: avoid "we", "I" in comments - they're often unclear. https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:34: // This constant is different for Chrome OS and everything else nit: no need to spell that out - the code gives it away :) https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:35: #ifdef OS_CHROMEOS nit: #if defined(OS_CHROMEOS) https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:38: auto setting_name = prefs::kAcceptLanguages; This bugs me. Do you know _why_ they're different? https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:91: "var count = 0;" Why not querySelectorAll? I.e. document.activeElement.querySelectorAll('.deletable-item').length;
lgtm
Note: First line of the description should match the subject.
I replaced the java script loop with a call to querySelectorAll and updated the comments for readability based on feedback. https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/language_options_interactive_uitest.cc (right): https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:25: // We need to emulate button pushing because we are testing accessibility, On 2014/08/18 21:42:04, groby wrote: > Nit: avoid "we", "I" in comments - they're often unclear. Done. https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:34: // This constant is different for Chrome OS and everything else On 2014/08/18 21:42:05, groby wrote: > nit: no need to spell that out - the code gives it away :) Done. https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:38: auto setting_name = prefs::kAcceptLanguages; Not sure why they're different. It may have to do with Chrome OS having more settings for languages than other OSs. On 2014/08/18 21:42:04, groby wrote: > This bugs me. Do you know _why_ they're different? https://codereview.chromium.org/464703002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:91: "var count = 0;" Did not know about querySelectorAll, updated code to take advantage of this. On 2014/08/18 21:42:04, groby wrote: > Why not querySelectorAll? > > I.e. document.activeElement.querySelectorAll('.deletable-item').length;
lgtm
The CQ bit was checked by hcarmona@chromium.org
The CQ bit was unchecked by hcarmona@chromium.org
lgtm https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/language_options_interactive_uitest.cc (right): https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:88: content::RenderFrameHost* active_frame = GetActiveFrame(); ASSERT_TRUE(active_frame); https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:96: EXPECT_TRUE(content::ExecuteScriptAndExtractInt( nit: ASSERT_TRUE() https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:105: nit: remove 1 \n https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:107: std::string languages = ""; nit: just std::string languages; https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:125: EXPECT_TRUE(PressKey(ui::VKEY_TAB)); nit: ASSERT_TRUE()
Hang on, I've got a bit more - the title is still super-generic. Please describe _what_ you fixed. I.e. "Fixed a11y tab-handling for language settings", or something that is a one-word description what this does.
Changed a EXPECT_TRUE to ASSERT_TRUE in locations where it doesn't make sense to continue the test. Removed unnecessary new line. https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/language_options_interactive_uitest.cc (right): https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:88: content::RenderFrameHost* active_frame = GetActiveFrame(); Not changed because test would have crashed by this point if it was not possible to get the active frame. On 2014/08/19 00:06:37, Dan Beam wrote: > ASSERT_TRUE(active_frame); https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:96: EXPECT_TRUE(content::ExecuteScriptAndExtractInt( On 2014/08/19 00:06:37, Dan Beam wrote: > nit: ASSERT_TRUE() Done. https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:105: On 2014/08/19 00:06:37, Dan Beam wrote: > nit: remove 1 \n Done. https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:107: std::string languages = ""; On 2014/08/19 00:06:37, Dan Beam wrote: > nit: just > > std::string languages; Done. https://codereview.chromium.org/464703002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_interactive_uitest.cc:125: EXPECT_TRUE(PressKey(ui::VKEY_TAB)); On 2014/08/19 00:06:37, Dan Beam wrote: > nit: ASSERT_TRUE() Done.
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hcarmona@chromium.org/464703002/160001
Message was sent while issue was closed.
Committed patchset #9 (160001) as 290651
Message was sent while issue was closed.
On 2014/08/19 20:16:43, I haz the power (commit-bot) wrote: > Committed patchset #9 (160001) as 290651 this test is flaking. see https://code.google.com/p/chromium/issues/detail?id=405711 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
