Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2096733002:
Fix the bug where focus jumps around on the media router sink list (Closed)
DescriptionFix the bug where focus jumps around on the media router sink list
The bug occurs when you are navigating the MR dialog on your keyboard.
When you search for a sink, focus on it using tab/arrow keys, and return
to the sink list with esc, the same sink ostensibly remains in focus,
but when you press up or down, the focus jumps to an unexpected place
because the focus had not been recognized on the Polymer side. This CL
fixes that by calling selectIndex, a Polymer paper-menu method for changing
the focus.
This CL also modifies a MR container filter test to check that the
appropriate sink not only remains in focus but also stays as the selected
item in the Polymer logic.
BUG=608578
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b20972c25ec11167e6cb77bf81a1573ea930d96a
Cr-Commit-Position: refs/heads/master@{#402866}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address imcheng@ and apacible@'s comments; modify a test #
Total comments: 3
Patch Set 3 : Address btolsch@ and apacible@'s comments #Patch Set 4 : Add selectIndex to externs.js #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== Fix the bug where focus jumps around on the media router sink list The bug occurs when you are navigating the MR dialog on your keyboard. When you search for a sink, focus on it using tab/arrow keys, and return to the sink list with esc, the same sink ostensibly remains in focus, but when you press up or down, the focus jumps to an unexpected place because the focus had not been recognized on the Polymer side. This CL fixes that by calling selectIndex, a Polymer paper-menu method for changing the focus. BUG=608578 ========== to ========== Fix the bug where focus jumps around on the media router sink list The bug occurs when you are navigating the MR dialog on your keyboard. When you search for a sink, focus on it using tab/arrow keys, and return to the sink list with esc, the same sink ostensibly remains in focus, but when you press up or down, the focus jumps to an unexpected place because the focus had not been recognized on the Polymer side. This CL fixes that by calling selectIndex, a Polymer paper-menu method for changing the focus. BUG=608578 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
takumif@chromium.org changed reviewers: + apacible@chromium.org, imcheng@chromium.org
Please take a look. Thank you!
lgtm Thanks for looking into this! I'd like Jennifer to take a look as well. https://codereview.chromium.org/2096733002/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2096733002/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1303: return true; nit: it looks like you can replace this statement with break; .
Thanks for looking into this! Can we add a test for this, so we don't accidentally cause a regression in the future? You can find our Polymer element tests here: chrome/test/data/webui/media_router/ https://codereview.chromium.org/2096733002/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2096733002/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1301: if (sinkList.itemForElement(sink).id == focusedSink.id) { nit: we can inline sinks[i] here.
Description was changed from ========== Fix the bug where focus jumps around on the media router sink list The bug occurs when you are navigating the MR dialog on your keyboard. When you search for a sink, focus on it using tab/arrow keys, and return to the sink list with esc, the same sink ostensibly remains in focus, but when you press up or down, the focus jumps to an unexpected place because the focus had not been recognized on the Polymer side. This CL fixes that by calling selectIndex, a Polymer paper-menu method for changing the focus. BUG=608578 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Fix the bug where focus jumps around on the media router sink list The bug occurs when you are navigating the MR dialog on your keyboard. When you search for a sink, focus on it using tab/arrow keys, and return to the sink list with esc, the same sink ostensibly remains in focus, but when you press up or down, the focus jumps to an unexpected place because the focus had not been recognized on the Polymer side. This CL fixes that by calling selectIndex, a Polymer paper-menu method for changing the focus. This CL also modifies a MR container filter test to check that the appropriate sink not only remains in focus but also stays as the selected item in the Polymer logic. BUG=608578 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
takumif@chromium.org changed reviewers: + btolsch@chromium.org
I've modified a test so that it doesn't pass without the fix in this CL. Please take a look, thank you! +btolsch@ for modifying a test he wrote.
lgtm + nit. Thanks for updating the test. https://codereview.chromium.org/2096733002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_filter_tests.js (right): https://codereview.chromium.org/2096733002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_filter_tests.js:197: // of the |container|. nit: The "selected" attribute is on the paper-menu, not |container|.
lgtm+nits https://codereview.chromium.org/2096733002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_filter_tests.js (right): https://codereview.chromium.org/2096733002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_filter_tests.js:195: // remains both focused and selected - focused refers to the "focused" nit: "...and selected, where focused refers to..." https://codereview.chromium.org/2096733002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_filter_tests.js:199: var FOCUS_INDEX = 2; nit: lower case.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org, apacible@chromium.org, btolsch@chromium.org Link to the patchset: https://codereview.chromium.org/2096733002/#ps40001 (title: "Address btolsch@ and apacible@'s comments")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by takumif@chromium.org
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
Try jobs failed on following builders: closure_compilation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org, apacible@chromium.org, btolsch@chromium.org Link to the patchset: https://codereview.chromium.org/2096733002/#ps60001 (title: "Add selectIndex to externs.js")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix the bug where focus jumps around on the media router sink list The bug occurs when you are navigating the MR dialog on your keyboard. When you search for a sink, focus on it using tab/arrow keys, and return to the sink list with esc, the same sink ostensibly remains in focus, but when you press up or down, the focus jumps to an unexpected place because the focus had not been recognized on the Polymer side. This CL fixes that by calling selectIndex, a Polymer paper-menu method for changing the focus. This CL also modifies a MR container filter test to check that the appropriate sink not only remains in focus but also stays as the selected item in the Polymer logic. BUG=608578 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Fix the bug where focus jumps around on the media router sink list The bug occurs when you are navigating the MR dialog on your keyboard. When you search for a sink, focus on it using tab/arrow keys, and return to the sink list with esc, the same sink ostensibly remains in focus, but when you press up or down, the focus jumps to an unexpected place because the focus had not been recognized on the Polymer side. This CL fixes that by calling selectIndex, a Polymer paper-menu method for changing the focus. This CL also modifies a MR container filter test to check that the appropriate sink not only remains in focus but also stays as the selected item in the Polymer logic. BUG=608578 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix the bug where focus jumps around on the media router sink list The bug occurs when you are navigating the MR dialog on your keyboard. When you search for a sink, focus on it using tab/arrow keys, and return to the sink list with esc, the same sink ostensibly remains in focus, but when you press up or down, the focus jumps to an unexpected place because the focus had not been recognized on the Polymer side. This CL fixes that by calling selectIndex, a Polymer paper-menu method for changing the focus. This CL also modifies a MR container filter test to check that the appropriate sink not only remains in focus but also stays as the selected item in the Polymer logic. BUG=608578 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Fix the bug where focus jumps around on the media router sink list The bug occurs when you are navigating the MR dialog on your keyboard. When you search for a sink, focus on it using tab/arrow keys, and return to the sink list with esc, the same sink ostensibly remains in focus, but when you press up or down, the focus jumps to an unexpected place because the focus had not been recognized on the Polymer side. This CL fixes that by calling selectIndex, a Polymer paper-menu method for changing the focus. This CL also modifies a MR container filter test to check that the appropriate sink not only remains in focus but also stays as the selected item in the Polymer logic. BUG=608578 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b20972c25ec11167e6cb77bf81a1573ea930d96a Cr-Commit-Position: refs/heads/master@{#402866} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b20972c25ec11167e6cb77bf81a1573ea930d96a Cr-Commit-Position: refs/heads/master@{#402866} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
