Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815623005:
MD Settings: in cr_dialog, prevent intersectionObserver from firing early. (Closed)
|
Created:
3 years, 8 months ago by scottchen Modified:
3 years, 7 months ago Reviewers:
dpapad CC:
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. |
DescriptionMD Settings: in cr_dialog, prevent intersectionObserver from firing early.
Currently the intersectionObesrver's handler gets triggered twice as cr-dialog
renders, reporting that it's intersecting with topMarker even though scrollTop
is 0. Delaying adding the observer until next render loop fixes the issue.
BUG=710355
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2815623005
Cr-Commit-Position: refs/heads/master@{#464617}
Committed: https://chromium.googlesource.com/chromium/src/+/fb69cfff512407b7d1e412a4a0ef71d9019daa8d
Patch Set 1 #
Total comments: 5
Patch Set 2 : check .isAttached and move intersectionObserver code to a method #Patch Set 3 : add/remove intersection listener by observing dialog.open changes #Patch Set 4 : add test #
Total comments: 2
Patch Set 5 : use mutationObserver to not mess with native dialog.open #
Total comments: 8
Patch Set 6 : updates based on comments #
Total comments: 2
Patch Set 7 : reduce indentation #
Messages
Total messages: 38 (21 generated)
Description was changed from ========== MD Settings: in cr_dialog, prevent intersectionObserver from firing early. BUG=710355 ========== to ========== MD Settings: in cr_dialog, prevent intersectionObserver from firing early. BUG=710355 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Currently the intersectionObesrver's handler gets triggered twice as cr-dialog renders, reporting that it's intersecting with topMarker even though scrollTop is 0. Delaying adding the observer until next render loop fixes the issue.
Description was changed from ========== MD Settings: in cr_dialog, prevent intersectionObserver from firing early. BUG=710355 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: in cr_dialog, prevent intersectionObserver from firing early. Currently the intersectionObesrver's handler gets triggered twice as cr-dialog renders, reporting that it's intersecting with topMarker even though scrollTop is 0. Delaying adding the observer until next render loop fixes the issue. BUG=710355 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
scottchen@chromium.org changed reviewers: + dpapad@chromium.org
The CQ bit was checked by scottchen@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/2815623005/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2815623005/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:94: Polymer.RenderStatus.afterNextRender(this, function() { Should we be calling observe() in ready() instead? According to the docs "Called after property values are set and local DOM is initialized."
https://codereview.chromium.org/2815623005/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2815623005/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:94: Polymer.RenderStatus.afterNextRender(this, function() { On 2017/04/12 01:05:51, dpapad wrote: > Should we be calling observe() in ready() instead? According to the docs > "Called after property values are set and local DOM is initialized." i'm fairly sure that attached() happens after ready().
https://codereview.chromium.org/2815623005/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2815623005/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:86: this.intersectionObserver_ = new IntersectionObserver( note: you might want to do this inside of your afterNextRender() as well as check whether this.isAttached in the afterNextRender callback
The CQ bit was checked by scottchen@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/2815623005/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2815623005/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:86: this.intersectionObserver_ = new IntersectionObserver( On 2017/04/12 01:09:43, Dan Beam wrote: > note: you might want to do this inside of your afterNextRender() as well as > check whether this.isAttached in the afterNextRender callback Done. Also moved the contained code to a separate method, nesting gettin' real. https://codereview.chromium.org/2815623005/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:94: Polymer.RenderStatus.afterNextRender(this, function() { On 2017/04/12 01:07:13, Dan Beam wrote: > On 2017/04/12 01:05:51, dpapad wrote: > > Should we be calling observe() in ready() instead? According to the docs > > "Called after property values are set and local DOM is initialized." > > i'm fairly sure that attached() happens after ready(). Yep, attached() fires after ready(). Not sure if it's due to how dialog gets rendered, but apparently when attached() gets called, only the overlay is visible, the actual modal is not yet.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I am a little concerned that we are addressing the problem at the wrong layer. Specifically: The description of this CL makes it sound that this is a problem of cr-dialog, but in fact I think it only happens when certain dialogs do async work in their attached() callback and modify the contents after they have been attached. Example https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/passwo.... So two questions: 1) Does this fix guarantee that the problem stops happening? Even if say a dialog yields twice in their attarched() callback before modifying the DOM? IIUC this fix assumes that waiting for one cycle is enough, but that is not guaranteed. 2) Could we potentially fix this by allowing a cr-dialog to change showScrollBorders attribute to true, only after it has finished rendering? This seems a bit more robust, no?
On 2017/04/12 17:25:17, dpapad wrote: > I am a little concerned that we are addressing the problem at the wrong layer. > Specifically: > > The description of this CL makes it sound that this is a problem of cr-dialog, > but in fact I think it only happens when certain dialogs do async work in their > attached() callback and modify the contents after they have been attached. > Example > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/passwo.... > > So two questions: > 1) Does this fix guarantee that the problem stops happening? Even if say a > dialog yields twice in their attarched() callback before modifying the DOM? IIUC > this fix assumes that waiting for one cycle is enough, but that is not > guaranteed. > > 2) Could we potentially fix this by allowing a cr-dialog to change > showScrollBorders attribute to true, only after it has finished rendering? This > seems a bit more robust, no? I've made another patch that moves all of these codes into an observer that watches for dialog.open changes, which will guarantee to be fired only after the modal is actually open.
The CQ bit was checked by scottchen@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I see some test failures. Will take another look after they've been fixed (assuming they are related to this CL). https://codereview.chromium.org/2815623005/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2815623005/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:48: observers: ['onOpenChanged_(open)'], You probably checked this already, but does this work without adding an "open" as a Polymer property within the "properties" section?
The CQ bit was checked by scottchen@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/2815623005/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2815623005/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:48: observers: ['onOpenChanged_(open)'], On 2017/04/13 01:12:12, dpapad wrote: > You probably checked this already, but does this work without adding an "open" > as a Polymer property within the "properties" section? It does work, but unfortunately found out that observing |open| messes with its native behavior. We started getting failing tests because dialog.open became ''/null instead of true/false after being observed. Submitted another patch to use MutationObserver instead.
Almost LG, see question below. https://codereview.chromium.org/2815623005/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right): https://codereview.chromium.org/2815623005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:157: var dialog = document.body.querySelector('dialog'); Just to make it explicit, let's also add assertFalse(dialog.open); https://codereview.chromium.org/2815623005/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2815623005/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:78: var observerConfig = { Nit (optional): Since this is only used once below, inline it instead? https://codereview.chromium.org/2815623005/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:92: } Previous code was removing the IntersectionObserver on detached, latest version does not. For example say someone is calling dialog.remove() without having called close() first. Would the IntersectionObserver leak? https://codereview.chromium.org/2815623005/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:123: Nit (optional): Revert this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2815623005/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right): https://codereview.chromium.org/2815623005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:157: var dialog = document.body.querySelector('dialog'); On 2017/04/13 20:31:39, dpapad wrote: > Just to make it explicit, let's also add > > assertFalse(dialog.open); Done. https://codereview.chromium.org/2815623005/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2815623005/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:78: var observerConfig = { On 2017/04/13 20:31:39, dpapad wrote: > Nit (optional): Since this is only used once below, inline it instead? Done. https://codereview.chromium.org/2815623005/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:92: } On 2017/04/13 20:31:39, dpapad wrote: > Previous code was removing the IntersectionObserver on detached, latest version > does not. For example say someone is calling dialog.remove() without having > called close() first. Would the IntersectionObserver leak? Yes, will leak. Fixed. https://codereview.chromium.org/2815623005/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:123: On 2017/04/13 20:31:39, dpapad wrote: > Nit (optional): Revert this? Done.
LGTM https://codereview.chromium.org/2815623005/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2815623005/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:95: addIntersectionObserver_: function() { Nit: You can reduce indentation throughout this method as follows if (this.intersectionObserver) return; // do stuff here....
https://codereview.chromium.org/2815623005/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2815623005/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:95: addIntersectionObserver_: function() { On 2017/04/13 22:02:03, dpapad wrote: > Nit: You can reduce indentation throughout this method as follows > > if (this.intersectionObserver) > return; > > // do stuff here.... Done.
The CQ bit was checked by scottchen@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/2815623005/#ps120001 (title: "reduce indentation")
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": 120001, "attempt_start_ts": 1492122826340230,
"parent_rev": "6fcd1789ae575456137efd8e707003492d443751", "commit_rev":
"fb69cfff512407b7d1e412a4a0ef71d9019daa8d"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: in cr_dialog, prevent intersectionObserver from firing early. Currently the intersectionObesrver's handler gets triggered twice as cr-dialog renders, reporting that it's intersecting with topMarker even though scrollTop is 0. Delaying adding the observer until next render loop fixes the issue. BUG=710355 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: in cr_dialog, prevent intersectionObserver from firing early. Currently the intersectionObesrver's handler gets triggered twice as cr-dialog renders, reporting that it's intersecting with topMarker even though scrollTop is 0. Delaying adding the observer until next render loop fixes the issue. BUG=710355 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2815623005 Cr-Commit-Position: refs/heads/master@{#464617} Committed: https://chromium.googlesource.com/chromium/src/+/fb69cfff512407b7d1e412a4a0ef... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/fb69cfff512407b7d1e412a4a0ef... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
