Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2944813002:
Use button elements instead of div for eject buttons. (Closed)
|
Created:
3 years, 6 months ago by tetsui Modified:
3 years, 6 months ago Reviewers:
yawano CC:
chromium-reviews, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yamaguchi+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse button elements instead of div for eject buttons.
This CL fixes the eject buttons of removable devices in FilesApp to use
button elements instead of div elements. It makes eject buttons focusable
by tab. Also ChromeVox recognizes it and says "Eject device - button".
BUG=732202
TEST=manually
* press tab repeatedly and press space on it.
* enable ChromeVox and use Search-Left/Right and Search-Down.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2944813002
Cr-Commit-Position: refs/heads/master@{#480732}
Committed: https://chromium.googlesource.com/chromium/src/+/22bdd8a52edc0fce8cd6ca7f90aa164cc59bc0a7
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move stylesheet to CSS file. #
Total comments: 2
Patch Set 3 : Fix CSS ordering. #
Messages
Total messages: 21 (14 generated)
Description was changed from ========== Use button element instead of div for eject buttons. This CL fixes the eject buttons of removable devices in FilesApp to use button elements instead of div elements. It makes eject button focusable by tab. Also ChromeVox recognizes it and says "Eject device - button". BUG=732202 TEST=manually * press tab repeatedly and press space on it. * enable ChromeVox and use Search-Left/Right and Search-Down. ========== to ========== Use button element instead of div for eject buttons. This CL fixes the eject buttons of removable devices in FilesApp to use button elements instead of div elements. It makes eject button focusable by tab. Also ChromeVox recognizes it and says "Eject device - button". BUG=732202 TEST=manually * press tab repeatedly and press space on it. * enable ChromeVox and use Search-Left/Right and Search-Down. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use button element instead of div for eject buttons. This CL fixes the eject buttons of removable devices in FilesApp to use button elements instead of div elements. It makes eject button focusable by tab. Also ChromeVox recognizes it and says "Eject device - button". BUG=732202 TEST=manually * press tab repeatedly and press space on it. * enable ChromeVox and use Search-Left/Right and Search-Down. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Use button elements instead of div for eject buttons. This CL fixes the eject buttons of removable devices in FilesApp to use button elements instead of div elements. It makes eject buttons focusable by tab. Also ChromeVox recognizes it and says "Eject device - button". BUG=732202 TEST=manually * press tab repeatedly and press space on it. * enable ChromeVox and use Search-Left/Right and Search-Down. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tetsui@chromium.org changed reviewers: + yawano@chromium.org
yawano@: PTAL. Thanks!
https://codereview.chromium.org/2944813002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/js/ui/directory_tree.js (right): https://codereview.chromium.org/2944813002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:668: ejectButton.setAttribute('style', 'border-style: none'); Move this to the css file. https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/...
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Thank you for reviewing! https://codereview.chromium.org/2944813002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/js/ui/directory_tree.js (right): https://codereview.chromium.org/2944813002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:668: ejectButton.setAttribute('style', 'border-style: none'); On 2017/06/20 01:44:44, yawano wrote: > Move this to the css file. > https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/... Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with a nit. https://codereview.chromium.org/2944813002/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/css/file_manager.css (right): https://codereview.chromium.org/2944813002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/css/file_manager.css:211: border-style: none; nit: order alphabetically. https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md...
Thanks! https://codereview.chromium.org/2944813002/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/css/file_manager.css (right): https://codereview.chromium.org/2944813002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/css/file_manager.css:211: border-style: none; On 2017/06/20 02:22:14, yawano wrote: > nit: order alphabetically. > https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md... Done.
The CQ bit was checked by tetsui@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yawano@chromium.org Link to the patchset: https://codereview.chromium.org/2944813002/#ps40001 (title: "Fix CSS ordering.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497925525500270,
"parent_rev": "37d0a33a1e1bf8f990938a6aba4afcb44af81de1", "commit_rev":
"3bb09b8dec0ca172c3bf11ca2256fcdfb0c938a5"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497925525500270,
"parent_rev": "e6e1307e43a8ce13ee9f47cade6eb658b90e44c0", "commit_rev":
"22bdd8a52edc0fce8cd6ca7f90aa164cc59bc0a7"}
Message was sent while issue was closed.
Description was changed from ========== Use button elements instead of div for eject buttons. This CL fixes the eject buttons of removable devices in FilesApp to use button elements instead of div elements. It makes eject buttons focusable by tab. Also ChromeVox recognizes it and says "Eject device - button". BUG=732202 TEST=manually * press tab repeatedly and press space on it. * enable ChromeVox and use Search-Left/Right and Search-Down. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Use button elements instead of div for eject buttons. This CL fixes the eject buttons of removable devices in FilesApp to use button elements instead of div elements. It makes eject buttons focusable by tab. Also ChromeVox recognizes it and says "Eject device - button". BUG=732202 TEST=manually * press tab repeatedly and press space on it. * enable ChromeVox and use Search-Left/Right and Search-Down. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2944813002 Cr-Commit-Position: refs/heads/master@{#480732} Committed: https://chromium.googlesource.com/chromium/src/+/22bdd8a52edc0fce8cd6ca7f90aa... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/22bdd8a52edc0fce8cd6ca7f90aa... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
