Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556243003:
IntersectionObserver: compute correct intersection ratio for 0x0 elements (Closed)
DescriptionIntersectionObserver: compute correct intersection ratio for 0x0 elements
When a 0x0 element is in view, its intersection ratio is defined[1] to
be 1.
BUG=662826
[1] https://wicg.github.io/IntersectionObserver/#update-intersection-observations-algo
Committed: https://crrev.com/8296d3b0e6a755dabf607b78a259f86803699d88
Cr-Commit-Position: refs/heads/master@{#437617}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Rewrite tests using testharness.js #Patch Set 3 : Script sorting #
Total comments: 2
Patch Set 4 : Logic fix #
Messages
Total messages: 27 (17 generated)
The CQ bit was checked by skyostil@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 ========== IntersectionObserver: compute correct intersection ratio for 0x0 elements When a 0x0 element is in view, its intersection ratio is defined[1] to be 1. BUG=662826 [1] https://wicg.github.io/IntersectionObserver/#update-intersection-observations... ========== to ========== IntersectionObserver: compute correct intersection ratio for 0x0 elements When a 0x0 element is in view, its intersection ratio is defined[1] to be 1. BUG=662826 [1] https://wicg.github.io/IntersectionObserver/#update-intersection-observations... ==========
skyostil@chromium.org changed reviewers: + foolip@chromium.org
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_...)
One-liner LGTM % testharness.js tests. https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-hidden.html (right): https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-hidden.html:2: <script src="../resources/js-test.js"></script> It looks like this test doesn't depend on any internal APIs, and intersection-observer-helper-functions.js doesn't either at a glance, can you write this using testharness.js for easier upstreaming to web-platform-tests later? https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-hidden.html:6: description("Ensure that a hidden zero-area element is treated correctly."); With testharness.js this can go in <title>, if there's just a single test in a file. https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-hidden.html:11: onload = function() { Does the test need to wait for the window load event, if so, it'll have to be: async_test(t => { onload = t.step_func(() => { // stuff }); }) (Otherwise one less nesting level is possible.) https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-hidden.html:16: waitForNotification(step0); With testharness.js, this would be: waitForNotification(t.step_func_done(() => { assert_equals(entries.length, 0); }); https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-visible-expected.txt (right): https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-visible-expected.txt:7: FAIL entries[0].intersectionRatio should be 1. Was 0. The bots disagree, did you forget to update expectations or should this indeed fail? https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-visible.html (right): https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-visible.html:21: if (entries.length > 0) { The above line says that it should be true, so no conditional needed? (With testharness.js, the test will stop at the first failed assert.)
ojan@google.com changed reviewers: + szager@chromium.org
Thanks for the testharness.js tip -- the tests look a lot cleaner this way. https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-hidden.html (right): https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-hidden.html:2: <script src="../resources/js-test.js"></script> On 2016/12/08 16:28:59, foolip wrote: > It looks like this test doesn't depend on any internal APIs, and > intersection-observer-helper-functions.js doesn't either at a glance, can you > write this using testharness.js for easier upstreaming to web-platform-tests > later? I'll give it a shot. This was just a carbon copy of an existing test here which explains some of the weirdness. https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-hidden.html:11: onload = function() { On 2016/12/08 16:28:59, foolip (OOO Dec. 9) wrote: > Does the test need to wait for the window load event, if so, it'll have to be: > > async_test(t => { > onload = t.step_func(() => { > // stuff > }); > }) > > (Otherwise one less nesting level is possible.) You're right -- there's no need to wait for anything to load here. https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-hidden.html:16: waitForNotification(step0); On 2016/12/08 16:28:59, foolip (OOO Dec. 9) wrote: > With testharness.js, this would be: > waitForNotification(t.step_func_done(() => { > assert_equals(entries.length, 0); > }); Done. https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-visible-expected.txt (right): https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-visible-expected.txt:7: FAIL entries[0].intersectionRatio should be 1. Was 0. On 2016/12/08 16:28:59, foolip (OOO Dec. 9) wrote: > The bots disagree, did you forget to update expectations or should this indeed > fail? Oops, fixed. https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-visible.html (right): https://codereview.chromium.org/2556243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/zero-area-element-visible.html:21: if (entries.length > 0) { On 2016/12/08 16:28:59, foolip (OOO Dec. 9) wrote: > The above line says that it should be true, so no conditional needed? (With > testharness.js, the test will stop at the first failed assert.) Very true, fixed. The existing tests had this guard for some reason...
The CQ bit was checked by skyostil@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/2556243003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2556243003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:56: newVisibleRatio = geometry.doesIntersect() ? 1 : 0; You also need to delete the preceding line, and instead use the same logic as for the non-degenerate case to get newThresholdIndex (i.e., observer().firstThresholdGreaterThan(newVisibleRatio)).
https://codereview.chromium.org/2556243003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2556243003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:56: newVisibleRatio = geometry.doesIntersect() ? 1 : 0; On 2016/12/09 17:30:07, szager1 wrote: > You also need to delete the preceding line, and instead use the same logic as > for the non-degenerate case to get newThresholdIndex (i.e., > observer().firstThresholdGreaterThan(newVisibleRatio)). Good catch. Rewrote this as we discussed offline.
The CQ bit was checked by skyostil@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...
lgtm, thanks.
The CQ bit was unchecked by skyostil@chromium.org
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2556243003/#ps60001 (title: "Logic fix")
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": 1481308664888300,
"parent_rev": "dff82afef7c099ce0ef008a3c79cba9d6540188d", "commit_rev":
"3e0c4f8fe580d9c1f46d6149fd51b2ee272ea205"}
Message was sent while issue was closed.
Description was changed from ========== IntersectionObserver: compute correct intersection ratio for 0x0 elements When a 0x0 element is in view, its intersection ratio is defined[1] to be 1. BUG=662826 [1] https://wicg.github.io/IntersectionObserver/#update-intersection-observations... ========== to ========== IntersectionObserver: compute correct intersection ratio for 0x0 elements When a 0x0 element is in view, its intersection ratio is defined[1] to be 1. BUG=662826 [1] https://wicg.github.io/IntersectionObserver/#update-intersection-observations... Review-Url: https://codereview.chromium.org/2556243003 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== IntersectionObserver: compute correct intersection ratio for 0x0 elements When a 0x0 element is in view, its intersection ratio is defined[1] to be 1. BUG=662826 [1] https://wicg.github.io/IntersectionObserver/#update-intersection-observations... Review-Url: https://codereview.chromium.org/2556243003 ========== to ========== IntersectionObserver: compute correct intersection ratio for 0x0 elements When a 0x0 element is in view, its intersection ratio is defined[1] to be 1. BUG=662826 [1] https://wicg.github.io/IntersectionObserver/#update-intersection-observations... Committed: https://crrev.com/8296d3b0e6a755dabf607b78a259f86803699d88 Cr-Commit-Position: refs/heads/master@{#437617} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8296d3b0e6a755dabf607b78a259f86803699d88 Cr-Commit-Position: refs/heads/master@{#437617} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
