Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2715793003:
Allow an element in shadow tree in <dialog> to be initially focused. (Closed)
DescriptionAllow an element in shadow tree in <dialog> to be initially focused.
This work arounds an issue when initally focused element on <dialog>
is in a shadow tree, by using FlatTreeTraversal rather than
NodeTraversal.
Note that this behavior may change in the future once the detailed
behavior is specified.
This change is originally from dbeam@ http://crrev.com/2695013010#ps1
BUG=383230
Review-Url: https://codereview.chromium.org/2715793003
Cr-Commit-Position: refs/heads/master@{#453541}
Committed: https://chromium.googlesource.com/chromium/src/+/1eaf120fcf6eff6426a8522de27eb3b204b1b5f9
Patch Set 1 #Patch Set 2 : Add another test case. #
Total comments: 3
Patch Set 3 : Update the comment. #
Messages
Total messages: 32 (23 generated)
The CQ bit was checked by kochi@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 ========== Allow Shadow DOM elements in <dialog> to be focused. This work arounds an issue when initally focused element on <dialog> is in a Shadow tree. Note that this behavior may change in the future once the spec specifies the detailed behavior, The original change is from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ========== to ========== Allow Shadow DOM elements in <dialog> to be focused. This work arounds an issue when initally focused element on <dialog> is in a Shadow tree. Note that this behavior may change in the future once the spec specifies the detailed behavior, The original change is from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ==========
kochi@chromium.org changed reviewers: + hayato@chromium.org
Description was changed from ========== Allow Shadow DOM elements in <dialog> to be focused. This work arounds an issue when initally focused element on <dialog> is in a Shadow tree. Note that this behavior may change in the future once the spec specifies the detailed behavior, The original change is from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ========== to ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a Shadow tree. Note that this behavior may change in the future once the spec specifies the detailed behavior. The original change is from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ==========
Description was changed from ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a Shadow tree. Note that this behavior may change in the future once the spec specifies the detailed behavior. The original change is from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ========== to ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a Shadow tree, by using FlatTreeTraversal rather than NodeTraversal. Note that this behavior may change in the future once the spec specifies the detailed behavior. The original change is from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ==========
Description was changed from ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a Shadow tree, by using FlatTreeTraversal rather than NodeTraversal. Note that this behavior may change in the future once the spec specifies the detailed behavior. The original change is from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ========== to ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a Shadow tree, by using FlatTreeTraversal rather than NodeTraversal. Note that this behavior may change in the future once the detailed behavior is specified. This chagne is originally from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ==========
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_...)
Description was changed from ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a Shadow tree, by using FlatTreeTraversal rather than NodeTraversal. Note that this behavior may change in the future once the detailed behavior is specified. This chagne is originally from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ========== to ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a shadow tree, by using FlatTreeTraversal rather than NodeTraversal. Note that this behavior may change in the future once the detailed behavior is specified. This chagne is originally from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ==========
PTAL
hayato@chromium.org changed reviewers: + dbeam@chromium.org
dbeam@, are you happy with this change as a workaround? Does this break anything?
I would change the specific implementation of focus in <dialog is="cr-dialog"> in MD settings, but I wouldn't be able to tell you if this now allows other components to be focused that we don't expect (that are deep in some shadow tree). Either way, I think doing this is worth a try. lgtm https://codereview.chromium.org/2715793003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right): https://codereview.chromium.org/2715793003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:48: // TODO(kochi): Use FlatTreeTraversal for finding focusable element in Shadow i think this is kinda of confusing, because it seems like (from reading this comment) that the code needs to be updated to use FlatTreeTraversal (but it already is). maybe something like: // Whether to use shadow DOM or not is currently unspecified. This may change at // any time. See crbug.com/383230 and https://github.com/whatwg/html/issue/2393.
Description was changed from ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a shadow tree, by using FlatTreeTraversal rather than NodeTraversal. Note that this behavior may change in the future once the detailed behavior is specified. This chagne is originally from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ========== to ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a shadow tree, by using FlatTreeTraversal rather than NodeTraversal. Note that this behavior may change in the future once the detailed behavior is specified. This change is originally from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ==========
The CQ bit was checked by kochi@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...
Patchset #3 (id:40001) has been deleted
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
I would -> meaning I'll update it
The CQ bit was checked by kochi@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/2715793003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right): https://codereview.chromium.org/2715793003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:48: // TODO(kochi): Use FlatTreeTraversal for finding focusable element in Shadow On 2017/02/27 04:16:25, Dan Beam wrote: > i think this is kinda of confusing, because it seems like (from reading this > comment) that the code needs to be updated to use FlatTreeTraversal (but it > already is). maybe something like: > > // Whether to use shadow DOM or not is currently unspecified. This may change at > // any time. See crbug.com/383230 and https://github.com/whatwg/html/issue/2393. Thanks for the feedback, updated the comment.
LGTM. I forgot to send a review comment. Could you confirm that? https://codereview.chromium.org/2715793003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right): https://codereview.chromium.org/2715793003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:51: for (Node* node = FlatTreeTraversal::firstChild(*dialog); node; node = next) { Distribution is not dirty here? If it is dirty, it would hit the assertion.
On 2017/02/28 06:12:36, hayato wrote: > LGTM. > > I forgot to send a review comment. Could you confirm that? > > https://codereview.chromium.org/2715793003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right): > > https://codereview.chromium.org/2715793003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:51: for (Node* node = > FlatTreeTraversal::firstChild(*dialog); node; node = next) { > Distribution is not dirty here? If it is dirty, it would hit the assertion. At this point of code layout is clean so that isFocusable() can work. So it implies distribution is clean.
The CQ bit was unchecked by kochi@chromium.org
The CQ bit was checked by kochi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2715793003/#ps60001 (title: "Update the comment.")
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": 60001, "attempt_start_ts": 1488263565261190,
"parent_rev": "2d0c679aff7cac0b26ca5c3652745db3393a817b", "commit_rev":
"1eaf120fcf6eff6426a8522de27eb3b204b1b5f9"}
Message was sent while issue was closed.
Description was changed from ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a shadow tree, by using FlatTreeTraversal rather than NodeTraversal. Note that this behavior may change in the future once the detailed behavior is specified. This change is originally from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 ========== to ========== Allow an element in shadow tree in <dialog> to be initially focused. This work arounds an issue when initally focused element on <dialog> is in a shadow tree, by using FlatTreeTraversal rather than NodeTraversal. Note that this behavior may change in the future once the detailed behavior is specified. This change is originally from dbeam@ http://crrev.com/2695013010#ps1 BUG=383230 Review-Url: https://codereview.chromium.org/2715793003 Cr-Commit-Position: refs/heads/master@{#453541} Committed: https://chromium.googlesource.com/chromium/src/+/1eaf120fcf6eff6426a8522de27e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1eaf120fcf6eff6426a8522de27e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
