Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2365743002:
[MR UI] Add a wrapper div to the sink list and search results in the MR dialog (Closed)
Description[MR UI] Add a wrapper div to the sink list and search results in the MR dialog
Instead of making the sink list/search results paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't trigger the _onFocus() callback on the paper-menu that puts focus on the first element in the menu and messes up the scroll position.
BUG=614844
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/0ca3ddbeaed0c6f6ee0f2a488a03abdeb8c64cae
Cr-Commit-Position: refs/heads/master@{#423347}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rename div wrapper, add a div wrapper for search results #
Total comments: 3
Patch Set 3 : Rebase, fix browser test #
Messages
Total messages: 35 (28 generated)
Description was changed from ========== . BUG= ========== to ========== . BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== . BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MR UI] Add a wrapper div to the sink list Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't fire the _onFocus() event on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [MR UI] Add a wrapper div to the sink list Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't fire the _onFocus() event on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MR UI] Add a wrapper div to the sink list Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't fire the _onFocus() event on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [MR UI] Add a wrapper div to the sink list Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't fire the _onFocus() event on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MR UI] Add a wrapper div to the sink list Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't fire the _onFocus() event on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. The only changes in the html file are the first two lines and the last line. The rest is all indentation. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [MR UI] Add a wrapper div to the sink list Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't fire the _onFocus() event on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. The only changes in the html file are the first two lines and the last line. The rest is all indentation. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MR UI] Add a wrapper div to the sink list in the MR dialog Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't fire the _onFocus() event on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. The only changes in the html file are the first two lines and the last line. The rest is all indentation. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [MR UI] Add a wrapper div to the sink list in the MR dialog Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't fire the _onFocus() event on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. The only changes in the html file are the first two lines and the last line. The rest is all indentation. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MR UI] Add a wrapper div to the sink list in the MR dialog Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't trigger the _onFocus() callback on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. The only changes in the html file are the first two lines and the last line. The rest is all indentation. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
takumif@chromium.org changed reviewers: + apacible@chromium.org
Please take a look, thank you!
https://codereview.chromium.org/2365743002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2365743002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:358: sinkListMaxHeight_: { Rename to |sinkListWrapperMaxHeight_|. https://codereview.chromium.org/2365743002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2349: sinkList ? this.computeElementVerticalPadding_(sinkList) : 0; If the wrapper exists, the sink list should also be stamped (regardless of device count). Do we need to differentiate between |sinkList| and |sinkListWrapper|?
Patchset #2 (id:40001) has been deleted
Description was changed from ========== [MR UI] Add a wrapper div to the sink list in the MR dialog Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't trigger the _onFocus() callback on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. The only changes in the html file are the first two lines and the last line. The rest is all indentation. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MR UI] Add a wrapper div to the sink list in the MR dialog Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't trigger the _onFocus() callback on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [MR UI] Add a wrapper div to the sink list in the MR dialog Instead of making the sink list paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't trigger the _onFocus() callback on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MR UI] Add a wrapper div to the sink list and search results in the MR dialog Instead of making the sink list/search results paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't trigger the _onFocus() callback on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Thanks for reviewing! I realized that the search results list also needs a wrapper, so I added that as well. https://codereview.chromium.org/2365743002/diff/20001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2365743002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:358: sinkListMaxHeight_: { On 2016/09/27 17:25:49, apacible wrote: > Rename to |sinkListWrapperMaxHeight_|. Reverted the name. https://codereview.chromium.org/2365743002/diff/20001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2349: sinkList ? this.computeElementVerticalPadding_(sinkList) : 0; On 2016/09/27 17:25:49, apacible wrote: > If the wrapper exists, the sink list should also be stamped (regardless of > device count). Do we need to differentiate between |sinkList| and > |sinkListWrapper|? No, we don't need to differentiate. https://codereview.chromium.org/2365743002/diff/60001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css (left): https://codereview.chromium.org/2365743002/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:157: * while animating. */ #search-results-container never actually was showing its scrollbar, since paper-menu#search-results within it had a max height set and had its own scrollbar. So its overflow-y should not change from hidden. We cannot give #search-results-container a max height, because that'd put the search input in the scrollable region. https://codereview.chromium.org/2365743002/diff/60001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (left): https://codereview.chromium.org/2365743002/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1863: resultsContainer.style['overflow-y'] = 'auto'; This wasn't doing anything, since the #search-results-container never has (nor should have) a scrollbar. https://codereview.chromium.org/2365743002/diff/60001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2365743002/diff/60001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1289: var sinkListPaperMenu = this.$$('#sink-list-paper-menu'); I've decided to change the names of the paper-menus and name the div wrappers #sink-list and #search-results. The CSS changes in this file that used to apply to the paper-menus should be applied to the div wrappers instead.
lgtm
The CQ bit was checked by takumif@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by takumif@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by takumif@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.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from apacible@chromium.org Link to the patchset: https://codereview.chromium.org/2365743002/#ps100001 (title: "Rebase, fix browser test")
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 ========== [MR UI] Add a wrapper div to the sink list and search results in the MR dialog Instead of making the sink list/search results paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't trigger the _onFocus() callback on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MR UI] Add a wrapper div to the sink list and search results in the MR dialog Instead of making the sink list/search results paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't trigger the _onFocus() callback on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [MR UI] Add a wrapper div to the sink list and search results in the MR dialog Instead of making the sink list/search results paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't trigger the _onFocus() callback on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MR UI] Add a wrapper div to the sink list and search results in the MR dialog Instead of making the sink list/search results paper-menu contain its own scrollbar, we add a wrapper div around it that contains a scrollbar. This way, clicking on the scrollbar doesn't trigger the _onFocus() callback on the paper-menu that puts focus on the first element in the menu and messes up the scroll position. BUG=614844 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0ca3ddbeaed0c6f6ee0f2a488a03abdeb8c64cae Cr-Commit-Position: refs/heads/master@{#423347} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0ca3ddbeaed0c6f6ee0f2a488a03abdeb8c64cae Cr-Commit-Position: refs/heads/master@{#423347} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
