Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2951703002:
[cr-action-menu] Fix anchoring to offscreen elements. (Closed)
|
Created:
3 years, 6 months ago by calamity Modified:
3 years, 5 months ago Reviewers:
dpapad CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, dbeam+watch-elements_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[cr-action-menu] Fix anchoring to offscreen elements.
This CL makes cr-action-menu anchor correctly to offscreen elements.
Previously, focusing the anchor element would shift the viewport, which
provided the wrong coordinates. By showing the dialog in the top-left, the
scroll is reset which correctly calculates the position of the dialog.
This CL also takes into account a scrolled body, which offsets the viewport.
BUG=734480
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2951703002
Cr-Commit-Position: refs/heads/master@{#483634}
Committed: https://chromium.googlesource.com/chromium/src/+/e57f5b9ac01324e8db26dbcdca46544832088a7d
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : fix closure #
Total comments: 4
Patch Set 5 : address comments #
Total comments: 6
Patch Set 6 : address test comments #Patch Set 7 : address comments #
Total comments: 2
Patch Set 8 : fix nit #
Dependent Patchsets: Messages
Total messages: 35 (20 generated)
Description was changed from ========== [cr-action-menu] Fix anchoring to offscreen elements. This CL makes cr-action-menu anchor correctly to offscreen elements. Previously, focusing the anchor element would shift the viewport, which provided the wrong coordinates. By showing the dialog in the top-left, the scroll is reset which correctly calculates the position of the dialog. This CL also takes into account a scrolled body, which offsets the viewport. BUG=734480 ========== to ========== [cr-action-menu] Fix anchoring to offscreen elements. This CL makes cr-action-menu anchor correctly to offscreen elements. Previously, focusing the anchor element would shift the viewport, which provided the wrong coordinates. By showing the dialog in the top-left, the scroll is reset which correctly calculates the position of the dialog. This CL also takes into account a scrolled body, which offsets the viewport. BUG=734480 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
calamity@chromium.org changed reviewers: + dpapad@chromium.org
On 2017/06/20 at 08:36:51, calamity wrote: > I am not able to apply this CL locally. Can you rebase?
Patchset #2 (id:20001) has been deleted
Sorry about that, done.
The CQ bit was checked by calamity@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: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
https://codereview.chromium.org/2951703002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2951703002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:332: this.style.top = '0'; Should this be '0px' ? https://codereview.chromium.org/2951703002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:340: var c = Object.assign(getDefaultShowConfig(), config); positionDialog_() is called from both showAtPosition() and showAt(). The latter already calls getDefaultShowConfig() and passes a config object that is fully populated. Can we do the same for showAtPostion() and stop calling Object.assign here. This way getDefaultShowConfig() will not called twice when someone uses showAt().
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2951703002/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2951703002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:332: this.style.top = '0'; On 2017/06/22 01:36:40, dpapad wrote: > Should this be '0px' ? It's convention for 0 to be used in place of 0px. https://codereview.chromium.org/2951703002/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:340: var c = Object.assign(getDefaultShowConfig(), config); On 2017/06/22 01:36:40, dpapad wrote: > positionDialog_() is called from both showAtPosition() and showAt(). The latter > already calls getDefaultShowConfig() and passes a config object that is fully > populated. Can we do the same for showAtPostion() and stop calling Object.assign > here. > > This way getDefaultShowConfig() will not called twice when someone uses > showAt(). Replaced the other call instead. This getDefaultShowConfig is nice here because it guarantees all the values accessed below will be defined.
https://codereview.chromium.org/2951703002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2951703002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:256: document.body.innerHTML = ` Could we pull out some of the CSS values into variables, and reuse them both in the innerHTML string literal here as well as in the assertions? https://codereview.chromium.org/2951703002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:319: document.body.scrollLeft = rect.right - document.body.clientWidth + 10; What is the magic 10 value here? https://codereview.chromium.org/2951703002/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2951703002/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:269: this.resetStyle_(); After patching this CL locally I discovered an undesirable behavior. See https://bugs.chromium.org/p/chromium/issues/detail?id=734480#c2. I think changing the viewport is causing the flicker.
The CQ bit was checked by calamity@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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The scrolling was happening because the wrong element was being scrolled back. Fixed. https://codereview.chromium.org/2951703002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2951703002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:256: document.body.innerHTML = ` On 2017/06/22 21:18:36, dpapad wrote: > Could we pull out some of the CSS values into variables, and reuse them both in > the innerHTML string literal here as well as in the assertions? Done. https://codereview.chromium.org/2951703002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:319: document.body.scrollLeft = rect.right - document.body.clientWidth + 10; On 2017/06/22 21:18:36, dpapad wrote: > What is the magic 10 value here? This was added just to make the test easier to visually debug (the button ended up behind the scrollbars. Removed. https://codereview.chromium.org/2951703002/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2951703002/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:269: this.resetStyle_(); On 2017/06/22 21:18:37, dpapad wrote: > After patching this CL locally I discovered an undesirable behavior. See > https://bugs.chromium.org/p/chromium/issues/detail?id=734480#c2. I think > changing the viewport is causing the flicker. Changed to use document.scrollingElement instead of document.body. The scrollingElement is <html> in downloads.
lgtm https://codereview.chromium.org/2951703002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2951703002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:275: PolymerTest.clearBody(); Nit: Since you are replacing innerHTML below, this is probably unnecessary.
https://codereview.chromium.org/2951703002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2951703002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:275: PolymerTest.clearBody(); On 2017/06/28 17:49:40, dpapad wrote: > Nit: Since you are replacing innerHTML below, this is probably unnecessary. Done.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2951703002/#ps180001 (title: "fix nit")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by calamity@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by calamity@chromium.org
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": 180001, "attempt_start_ts": 1498791127052160,
"parent_rev": "8e7f9967cab8bb7eba9af7f29d63c69dff2c72b5", "commit_rev":
"e57f5b9ac01324e8db26dbcdca46544832088a7d"}
Message was sent while issue was closed.
Description was changed from ========== [cr-action-menu] Fix anchoring to offscreen elements. This CL makes cr-action-menu anchor correctly to offscreen elements. Previously, focusing the anchor element would shift the viewport, which provided the wrong coordinates. By showing the dialog in the top-left, the scroll is reset which correctly calculates the position of the dialog. This CL also takes into account a scrolled body, which offsets the viewport. BUG=734480 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [cr-action-menu] Fix anchoring to offscreen elements. This CL makes cr-action-menu anchor correctly to offscreen elements. Previously, focusing the anchor element would shift the viewport, which provided the wrong coordinates. By showing the dialog in the top-left, the scroll is reset which correctly calculates the position of the dialog. This CL also takes into account a scrolled body, which offsets the viewport. BUG=734480 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2951703002 Cr-Commit-Position: refs/heads/master@{#483634} Committed: https://chromium.googlesource.com/chromium/src/+/e57f5b9ac01324e8db26dbcdca46... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/e57f5b9ac01324e8db26dbcdca46... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
