Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2465433002:
Create implementation of the side panel using a dialog. (Closed)
|
Created:
4 years, 1 month ago by hcarmona Modified:
4 years ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-elements_chromium.org, dbeam+watch-closure_chromium.org, dbeam+watch-settings_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-elements_chromium.org, michaelpg+watch-md-ui_chromium.org, michaelpg+watch-md-settings_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, tsergeant, vitalyp+closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate implementation of the side panel using a dialog.
Using a dialog is a much cleaner solution because it handles all focus
trapping without attempting to capture TAB or require updating all
tabindex properties.
We removed the ability to swipe the drawer open in order to keep the
code simple.
BUG=633858
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/bae4924a123c42de82f4fef429cc176ee9065785
Cr-Commit-Position: refs/heads/master@{#438187}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Feedback #
Total comments: 4
Patch Set 3 : fix test #Patch Set 4 : Address feedback. #
Total comments: 6
Patch Set 5 : feedback #
Total comments: 8
Patch Set 6 : nits #
Total comments: 3
Patch Set 7 : nit #Messages
Total messages: 75 (58 generated)
Description was changed from ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. BUG=605108 ========== to ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. BUG=605108 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@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: linux_chromium_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 hcarmona@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by hcarmona@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 #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. BUG=605108 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. BUG=633858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@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.
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
hcarmona@chromium.org changed reviewers: + dpapad@chromium.org
PTAL
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.
This is a big CL, trying to understand the big picture first. Sending some initial questions to understand the overall design, before diving into details. Also why is the new element in cr_elements, instead of settings/. Generally I think it is better to promote an element to cr_elements/ once a 2nd user (WebUI page) is found. This prevents cr_elements/ to be populated with elements that are only used in a single WebUI page, or elements that are advertized as "generic" but in practice are very tied to the only WebUI page currently using them. https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html (right): https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html:89: <dialog id="contentContainer" on-tap="onDialogTap_"> Why do we need to wrap <dialog> instead of extending one (similar to how cr-dialog and cr-action-menu do)? Why do we need a "scrim" div? Native dialog comes with its own scrim (which is configurable with ::backdrop CSS pseudoclass. https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_drawer/cr_drawer.js (right): https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_drawer/cr_drawer.js:50: translateOffset_: 0, Can you document this member var? Both its type but also a short comment about what info it holds. Same for trackDetails_ and drawerState_. https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_drawer/cr_drawer.js:336: setTransitionDuration_: function(duration) { Let's annotate all methods in this class with @param, @private, @return where applicable.
https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html (right): https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html:89: <dialog id="contentContainer" on-tap="onDialogTap_"> On 2016/11/07 at 20:13:24, dpapad wrote: > Why do we need to wrap <dialog> instead of extending one (similar to how cr-dialog and cr-action-menu do)? > > Why do we need a "scrim" div? Native dialog comes with its own scrim (which is configurable with ::backdrop CSS pseudoclass. FYI, I put together a minimal example at https://jsfiddle.net/1y68t3rj/4/. I did not add functionality to drag the dialog with the mouse, but as a proof of concept it is possible to use the ::backdrop instead of a separate scrim.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
i don't think we need to basically copy app-drawer. can we just do a simple css transition without gestures / canceling for now?
The CQ bit was checked by hcarmona@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...
Description was changed from ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. BUG=633858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. In order to keep the code simple, we removed the ability to slide the drawer open. BUG=633858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. In order to keep the code simple, we removed the ability to slide the drawer open. BUG=633858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. We removed the ability to swipe the drawer open in order to keep the code simple. BUG=633858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Thanks for all the feedback, this is now much simpler :-) Also moved out of cr_elements. https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html (right): https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html:89: <dialog id="contentContainer" on-tap="onDialogTap_"> On 2016/11/07 20:13:24, dpapad wrote: > Why do we need to wrap <dialog> instead of extending one (similar to how > cr-dialog and cr-action-menu do)? Changed to extend dialog. > Why do we need a "scrim" div? Native dialog comes with its own scrim (which is > configurable with ::backdrop CSS pseudoclass. Scrim is gone now. :-) https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html:89: <dialog id="contentContainer" on-tap="onDialogTap_"> On 2016/11/08 20:34:05, dpapad wrote: > On 2016/11/07 at 20:13:24, dpapad wrote: > > Why do we need to wrap <dialog> instead of extending one (similar to how > cr-dialog and cr-action-menu do)? > > > > Why do we need a "scrim" div? Native dialog comes with its own scrim (which is > configurable with ::backdrop CSS pseudoclass. > > FYI, I put together a minimal example at https://jsfiddle.net/1y68t3rj/4/. I did > not add functionality to drag the dialog with the mouse, but as a proof of > concept it is possible to use the ::backdrop instead of a separate scrim. Thanks! That was super useful, new patch uses the ::backdrop. https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_drawer/cr_drawer.js (right): https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_drawer/cr_drawer.js:50: translateOffset_: 0, On 2016/11/07 20:13:24, dpapad wrote: > Can you document this member var? Both its type but also a short comment about > what info it holds. Same for trackDetails_ and drawerState_. Done. https://codereview.chromium.org/2465433002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_drawer/cr_drawer.js:336: setTransitionDuration_: function(duration) { On 2016/11/07 20:13:24, dpapad wrote: > Let's annotate all methods in this class with @param, @private, @return where > applicable. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
That looks so much simpler! Found a small issue though. If you open the drawer and navigate to a section. The next time you open it, it does not animate. https://codereview.chromium.org/2465433002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.js (right): https://codereview.chromium.org/2465433002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:39: open_: function() { @private. Here and elsewhere. https://codereview.chromium.org/2465433002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:62: if (event.detail.x < rect.left || event.detail.x > rect.right) Would this also work, given that the dialog is filled with contents (drawer-header, drawer-contents)? if (e.target == this) this.close_();
The CQ bit was checked by hcarmona@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 hcarmona@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...
Fixed the test and addressed feedback. Also fixed the issue dpapad@ mentioned about animations breaking after clicking a link in the side panel. https://codereview.chromium.org/2465433002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.js (right): https://codereview.chromium.org/2465433002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:39: open_: function() { On 2016/11/16 22:52:46, dpapad wrote: > @private. Here and elsewhere. This is public now. https://codereview.chromium.org/2465433002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:62: if (event.detail.x < rect.left || event.detail.x > rect.right) On 2016/11/16 22:52:46, dpapad wrote: > Would this also work, given that the dialog is filled with contents > (drawer-header, drawer-contents)? > > if (e.target == this) > this.close_(); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2465433002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.html (right): https://codereview.chromium.org/2465433002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.html:18: width: 256px; What is the relation of 256 and 265 above? Should this be 265 too? Also can we use a CSS variable wherevere possible, to avoid repeating literal values? https://codereview.chromium.org/2465433002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.js (right): https://codereview.chromium.org/2465433002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:63: var rect = this.getBoundingClientRect(); We can avoid the need to call getBoundingClientRect() as shown at http://pastebin.com/q8fRpG1L. I tried it locally and seemed to work equivalently. getBoundingClientRect() forces a re-layout, but also has a some bugs that have not been fixed (crbug.comm/662300 and crbug.com/667198). https://codereview.chromium.org/2465433002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:86: this.fire('open'); // Notify tests that dialog is now open. Can we rename the event to something more specific? dialog-drawer-open maybe?
The CQ bit was checked by hcarmona@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...
https://codereview.chromium.org/2465433002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.html (right): https://codereview.chromium.org/2465433002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.html:18: width: 256px; On 2016/12/08 22:27:22, dpapad wrote: > What is the relation of 256 and 265 above? Should this be 265 too? > > Also can we use a CSS variable wherevere possible, to avoid repeating literal > values? Good catch! Yes, both values should be the same. Updated to use a variable. https://codereview.chromium.org/2465433002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.js (right): https://codereview.chromium.org/2465433002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:63: var rect = this.getBoundingClientRect(); On 2016/12/08 22:27:22, dpapad wrote: > We can avoid the need to call getBoundingClientRect() as shown at > http://pastebin.com/q8fRpG1L. > > I tried it locally and seemed to work equivalently. getBoundingClientRect() > forces a re-layout, but also has a some bugs that have not been fixed > (crbug.comm/662300 and crbug.com/667198). Good to know. Done. https://codereview.chromium.org/2465433002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:86: this.fire('open'); // Notify tests that dialog is now open. On 2016/12/08 22:27:22, dpapad wrote: > Can we rename the event to something more specific? dialog-drawer-open maybe? As per our conversation: I've removed the event and the test listens for transitionend.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits. https://codereview.chromium.org/2465433002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.html (right): https://codereview.chromium.org/2465433002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.html:17: transition: left 200ms ease; Nit: perhaps you could also add one more variable, as follows? --transition-timing: 200ms ease; And use it as follows transition: left var(--transition-timing); transition: right var(--transition-timing); transition: opacity var(--transition-timing); https://codereview.chromium.org/2465433002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.js (right): https://codereview.chromium.org/2465433002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:57: * @param {Event} event !Event here and line 75. https://codereview.chromium.org/2465433002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:69: onDialogTap_: function(event) { We are not using |event| here, so how about dropping it completely? https://codereview.chromium.org/2465433002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_ui_browsertest.js (right): https://codereview.chromium.org/2465433002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_ui_browsertest.js:68: 'concelable': true, s/concelable/cancelable
The CQ bit was checked by hcarmona@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...
Fixed nits. dbeam@, any feedback before landing this? https://codereview.chromium.org/2465433002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.html (right): https://codereview.chromium.org/2465433002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.html:17: transition: left 200ms ease; On 2016/12/09 18:57:53, dpapad wrote: > Nit: perhaps you could also add one more variable, as follows? > > --transition-timing: 200ms ease; > > And use it as follows > transition: left var(--transition-timing); > transition: right var(--transition-timing); > transition: opacity var(--transition-timing); Done. https://codereview.chromium.org/2465433002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.js (right): https://codereview.chromium.org/2465433002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:57: * @param {Event} event On 2016/12/09 18:57:53, dpapad wrote: > !Event here and line 75. Done. https://codereview.chromium.org/2465433002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.js:69: onDialogTap_: function(event) { On 2016/12/09 18:57:53, dpapad wrote: > We are not using |event| here, so how about dropping it completely? Done. https://codereview.chromium.org/2465433002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_ui_browsertest.js (right): https://codereview.chromium.org/2465433002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_ui_browsertest.js:68: 'concelable': true, On 2016/12/09 18:57:53, dpapad wrote: > s/concelable/cancelable Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2465433002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.html (right): https://codereview.chromium.org/2465433002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.html:9: background-color: #FFF; nit: #fff (lower case, what HTML/CSS style guide recommends) or just "white" https://codereview.chromium.org/2465433002/diff/180001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_ui_browsertest.js (right): https://codereview.chromium.org/2465433002/diff/180001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_ui_browsertest.js:65: var midScreen = MockInteractions.middleOfNode(ui); TIL: middleOfNode
btw, this is a super clean up!
The CQ bit was checked by hcarmona@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...
Thanks for all the feedback! Landing now :-) https://codereview.chromium.org/2465433002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/controls/dialog_drawer.html (right): https://codereview.chromium.org/2465433002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/controls/dialog_drawer.html:9: background-color: #FFF; On 2016/12/13 03:59:28, Dan Beam wrote: > nit: #fff (lower case, what HTML/CSS style guide recommends) or just "white" Done.
The CQ bit was unchecked by hcarmona@chromium.org
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2465433002/#ps200001 (title: "nit")
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": 200001, "attempt_start_ts": 1481643382498600,
"parent_rev": "01a8ce8a7bcdee9d4072b6ad29e446207b9844f8", "commit_rev":
"c9880e9d91e611b4a9a2644fa9f3a9421263f445"}
Message was sent while issue was closed.
Description was changed from ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. We removed the ability to swipe the drawer open in order to keep the code simple. BUG=633858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. We removed the ability to swipe the drawer open in order to keep the code simple. BUG=633858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2465433002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. We removed the ability to swipe the drawer open in order to keep the code simple. BUG=633858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2465433002 ========== to ========== Create implementation of the side panel using a dialog. Using a dialog is a much cleaner solution because it handles all focus trapping without attempting to capture TAB or require updating all tabindex properties. We removed the ability to swipe the drawer open in order to keep the code simple. BUG=633858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/bae4924a123c42de82f4fef429cc176ee9065785 Cr-Commit-Position: refs/heads/master@{#438187} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/bae4924a123c42de82f4fef429cc176ee9065785 Cr-Commit-Position: refs/heads/master@{#438187} |
