Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10977032:
Screen reader now reads the file manager selection. (Closed)
|
Created:
8 years, 2 months ago by mtomasz Modified:
8 years, 2 months ago Reviewers:
Dominic at Gmail (DO NOT USE), arv (Not doing code reviews), kaznacheev, Vladislav Kaznacheev, James Hawkins, Dmitry Zvorygin CC:
chromium-reviews, rginda+watch_chromium.org, arv (Not doing code reviews), Zachary Kuznia Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionScreen reader now reads the file manager selection.
Screen reader (Chrome Vox) on ChromeOS does not work correctly in File Manager. This patch makes the screen reader read file selection details, when the selection is changed.
BUG=151471
TEST=Enable Screen Reader, enter File Manager and click on several files to invoke Chrome Vox reading.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159677
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebased. #
Total comments: 5
Patch Set 3 : Fixed a comment. #Patch Set 4 : Fixer components order.' #
Messages
Total messages: 23 (0 generated)
Screen reader now reads the file manager selection. Screen reader (Chrome Vox) on ChromeOS does not work correctly in File Manager. This patch makes the screen reader read file selection details, when the selection is changed. BUG=151471 TEST=Enable Screen Reader, enter File Manager and click on several files to invoke Chrome Vox reading.
PTAL
LGTM chrome/browser/resources/file_manager/main.html
http://codereview.chromium.org/10977032/diff/1/chrome/browser/resources/file_... File chrome/browser/resources/file_manager/main.html (right): http://codereview.chromium.org/10977032/diff/1/chrome/browser/resources/file_... chrome/browser/resources/file_manager/main.html:226: <div class=detail-table id="detail-table" tabindex=0></div> What for do you need id here?
On 2012/09/26 11:57:31, Dmitry Zvorygin wrote: > http://codereview.chromium.org/10977032/diff/1/chrome/browser/resources/file_... > File chrome/browser/resources/file_manager/main.html (right): > > http://codereview.chromium.org/10977032/diff/1/chrome/browser/resources/file_... > chrome/browser/resources/file_manager/main.html:226: <div class=detail-table > id="detail-table" tabindex=0></div> > What for do you need id here? This bug has just been fixed by @aboxhall, however this patch covers more, eg. fixes setting of id prefix for cr.ui.Table's items.
http://codereview.chromium.org/10977032/diff/1/chrome/browser/resources/file_... File chrome/browser/resources/file_manager/main.html (right): http://codereview.chromium.org/10977032/diff/1/chrome/browser/resources/file_... chrome/browser/resources/file_manager/main.html:226: <div class=detail-table id="detail-table" tabindex=0></div> On 2012/09/26 11:57:31, Dmitry Zvorygin wrote: > What for do you need id here? This is essential to build ids for cr.ui.ListItems. Otherwise we will have a collision, since grid and list will have the same prefix (list-container here).
On 2012/09/26 12:04:47, mtomasz wrote: > http://codereview.chromium.org/10977032/diff/1/chrome/browser/resources/file_... > File chrome/browser/resources/file_manager/main.html (right): > > http://codereview.chromium.org/10977032/diff/1/chrome/browser/resources/file_... > chrome/browser/resources/file_manager/main.html:226: <div class=detail-table > id="detail-table" tabindex=0></div> > On 2012/09/26 11:57:31, Dmitry Zvorygin wrote: > > What for do you need id here? > > This is essential to build ids for cr.ui.ListItems. Otherwise we will have a > collision, since grid and list will have the same prefix (list-container here). Ping.
On 2012/09/28 02:34:51, mtomasz wrote: > On 2012/09/26 12:04:47, mtomasz wrote: > > > http://codereview.chromium.org/10977032/diff/1/chrome/browser/resources/file_... > > File chrome/browser/resources/file_manager/main.html (right): > > > > > http://codereview.chromium.org/10977032/diff/1/chrome/browser/resources/file_... > > chrome/browser/resources/file_manager/main.html:226: <div class=detail-table > > id="detail-table" tabindex=0></div> > > On 2012/09/26 11:57:31, Dmitry Zvorygin wrote: > > > What for do you need id here? > > > > This is essential to build ids for cr.ui.ListItems. Otherwise we will have a > > collision, since grid and list will have the same prefix (list-container > here). > > Ping. It was just a question. You have got already LGTM from our team(kaznacheev) for main.html
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/10977032/5001
Presubmit check for 10977032-5001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/resources
Presubmit checks took 4.3s to calculate.
On 2012/09/28 11:52:07, I haz the power (commit-bot) wrote: > Presubmit check for 10977032-5001 failed and returned exit status 1. > > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for files in these directories: > chrome/browser/resources > > Presubmit checks took 4.3s to calculate. I'm still missing an LGTM for chrome/browser/resources. Sorry, seems that I've missed it.
@arv, @jhawkins: PTAL.
On 2012/09/28 11:56:40, mtomasz wrote: > @arv, @jhawkins: PTAL. @arv, @jhawkins: Ping, why so long? It is a tiny CL.
On 2012/10/01 01:44:56, mtomasz wrote: > On 2012/09/28 11:56:40, mtomasz wrote: > > @arv, @jhawkins: PTAL. > > @arv, @jhawkins: > Ping, why so long? It is a tiny CL. Perf?
http://codereview.chromium.org/10977032/diff/5001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/table.js (right): http://codereview.chromium.org/10977032/diff/5001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/table.js:177: this.appendChild(this.header_); Why did you make this change? http://codereview.chromium.org/10977032/diff/5001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/table/table_list.js (right): http://codereview.chromium.org/10977032/diff/5001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/table/table_list.js:75: // cr.ui.Table. We must not access `this` here, since it may be anything. nit: Don't use pronouns (we) in comments; pronouns are ambiguous. Really though, I don't think the comment is necessary. Subjectivity such as this is not appropriate in comments.
http://codereview.chromium.org/10977032/diff/5001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/table.js (right): http://codereview.chromium.org/10977032/diff/5001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/table.js:177: this.appendChild(this.header_); On 2012/10/01 02:12:56, James Hawkins wrote: > Why did you make this change? We have to append the list to the DOM tree before we decorate it, because list decorate method traverses DOM tree to find the first ancestor with set id. http://codereview.chromium.org/10977032/diff/5001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/table/table_list.js (right): http://codereview.chromium.org/10977032/diff/5001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/table/table_list.js:75: // cr.ui.Table. We must not access `this` here, since it may be anything. On 2012/10/01 02:12:56, James Hawkins wrote: > nit: Don't use pronouns (we) in comments; pronouns are ambiguous. > > Really though, I don't think the comment is necessary. Subjectivity such as > this is not appropriate in comments. Subjectivity? We must not access `this`, this is essential and very bug prone. And also, accessing and modifying private members from outside is acceptable? I've fixed the comment, though a little.
LGTM http://codereview.chromium.org/10977032/diff/5001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/table/table_list.js (right): http://codereview.chromium.org/10977032/diff/5001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/table/table_list.js:75: // cr.ui.Table. We must not access `this` here, since it may be anything. On 2012/10/01 02:50:55, mtomasz wrote: > On 2012/10/01 02:12:56, James Hawkins wrote: > > nit: Don't use pronouns (we) in comments; pronouns are ambiguous. > > > > Really though, I don't think the comment is necessary. Subjectivity such as > > this is not appropriate in comments. > > Subjectivity? We must not access `this`, this is essential and very bug prone. > And also, accessing and modifying private members from outside is acceptable? > I've fixed the comment, though a little. The part you removed was subjective. Thanks for fixing it up.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/10977032/13002
Retried try job too often for step(s) crypto_unittests, unit_tests, cacheinvalidation_unittests, remoting_unittests, jingle_unittests, nacl_integration, ipc_tests, interactive_ui_tests, browser_tests, net_unittests, content_unittests, gpu_unittests, base_unittests, sync_integration_tests, media_unittests, safe_browsing_tests, content_browsertests, sql_unittests, printing_unittests, sync_unit_tests, check_deps
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/10977032/13002
Retried try job too often for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/10977032/13002
Change committed as 159677 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
