[css-scroll-snap] ScrollSnapData not initialized correctly on cc |
|||
Issue descriptionIn a recent change, we added some fields in SnapContainerData, but didn't initialize them correctly in the copy constructor and move constructor, causing the scrollers unable to snap when the page is composited. This was undiscovered because the automation of the test snap-at-user-scroll-end-manual.html is outdated and is not examining the composited case.
,
Jul 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0fc70abb02f52598f0f98eafd7001490545bc0c9 commit 0fc70abb02f52598f0f98eafd7001490545bc0c9 Author: Taiju Tsuiki <tzik@chromium.org> Date: Fri Jul 13 03:39:05 2018 Revert "Correctly initialize and test SnapContainerData in cc." This reverts commit 37061f875b167ab82f1a0ddca84b510b7c6802dc. Reason for revert: The layout tests added by this CL is failing on the CL: https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Win10/37804 Original change's description: > Correctly initialize and test SnapContainerData in cc. > > This patch initializes the rect_ field in SnapContainerData in copy > constructor and move constructor. Fixing a snapping issue in the > composited pages. > > We also adds external/wpt/css/css-scroll-snap/ to the virtual/threaded > test suite so that they are tested with the composited cases. > > snap-at-user-scroll-end-manual-automation.js calls > mouseClickInTarget() of pointerevent_common_input.js. That method > invokes programmatic scrolls. The test was written before programmatic > scroll snapping was implemented so it worked at that time. However, > with programmatic scroll snapping implemented, it will always snap > in mouseClickIntarget() and cannot test whether the user scroll snaps. > This patch adds a parameter shouldScrollToTarget to mouseClickInTarget > to avoid invoking programmatic scrolls in the test. > > This patch also checks nullptr for layout_box in > ScrollManager::SnapAtGestureScrollEnd() to fix a crash. > > Bug: 862406 , 862571 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel > Change-Id: I6a53818cf74421a4100ad6f908158abf302a5b8e > Reviewed-on: https://chromium-review.googlesource.com/1132386 > Commit-Queue: Sandra Sun <sunyunjia@chromium.org> > Reviewed-by: Majid Valipour <majidvp@chromium.org> > Reviewed-by: Robert Flack <flackr@chromium.org> > Cr-Commit-Position: refs/heads/master@{#574781} TBR=flackr@chromium.org,majidvp@chromium.org,sunyunjia@chromium.org Change-Id: I55c77c05c0381c8ac638bd106d2d18b1b4332745 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 862406 , 862571 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Reviewed-on: https://chromium-review.googlesource.com/1136171 Reviewed-by: Taiju Tsuiki <tzik@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#574825} [modify] https://crrev.com/0fc70abb02f52598f0f98eafd7001490545bc0c9/cc/input/scroll_snap_data.cc [modify] https://crrev.com/0fc70abb02f52598f0f98eafd7001490545bc0c9/third_party/WebKit/LayoutTests/VirtualTestSuites [modify] https://crrev.com/0fc70abb02f52598f0f98eafd7001490545bc0c9/third_party/WebKit/LayoutTests/external/wpt/css/css-scroll-snap/snap-at-user-scroll-end-manual.html [modify] https://crrev.com/0fc70abb02f52598f0f98eafd7001490545bc0c9/third_party/WebKit/LayoutTests/external/wpt_automation/css/css-scroll-snap/snap-at-user-scroll-end-manual-automation.js [modify] https://crrev.com/0fc70abb02f52598f0f98eafd7001490545bc0c9/third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_common_input.js [delete] https://crrev.com/af7eb0ad7bfded346662f7373d4a207d9298d299/third_party/WebKit/LayoutTests/virtual/threaded/external/wpt/css/css-scroll-snap/README.txt [modify] https://crrev.com/0fc70abb02f52598f0f98eafd7001490545bc0c9/third_party/blink/renderer/core/input/scroll_manager.cc
,
Jul 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/043b97cee41154a32d6c1cd8965075813a9ba672 commit 043b97cee41154a32d6c1cd8965075813a9ba672 Author: Sandra Sun <sunyunjia@chromium.org> Date: Fri Jul 13 15:44:20 2018 Reland "Correctly initialize and test SnapContainerData in cc." This reverts commit 0fc70abb02f52598f0f98eafd7001490545bc0c9. Reason for revert: <INSERT REASONING HERE> Original change's description: > Revert "Correctly initialize and test SnapContainerData in cc." > > This reverts commit 37061f875b167ab82f1a0ddca84b510b7c6802dc. > > Reason for revert: > The layout tests added by this CL is failing on the CL: > https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Win10/37804 > > Original change's description: > > Correctly initialize and test SnapContainerData in cc. > > > > This patch initializes the rect_ field in SnapContainerData in copy > > constructor and move constructor. Fixing a snapping issue in the > > composited pages. > > > > We also adds external/wpt/css/css-scroll-snap/ to the virtual/threaded > > test suite so that they are tested with the composited cases. > > > > snap-at-user-scroll-end-manual-automation.js calls > > mouseClickInTarget() of pointerevent_common_input.js. That method > > invokes programmatic scrolls. The test was written before programmatic > > scroll snapping was implemented so it worked at that time. However, > > with programmatic scroll snapping implemented, it will always snap > > in mouseClickIntarget() and cannot test whether the user scroll snaps. > > This patch adds a parameter shouldScrollToTarget to mouseClickInTarget > > to avoid invoking programmatic scrolls in the test. > > > > This patch also checks nullptr for layout_box in > > ScrollManager::SnapAtGestureScrollEnd() to fix a crash. > > > > Bug: 862406 , 862571 > > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel > > Change-Id: I6a53818cf74421a4100ad6f908158abf302a5b8e > > Reviewed-on: https://chromium-review.googlesource.com/1132386 > > Commit-Queue: Sandra Sun <sunyunjia@chromium.org> > > Reviewed-by: Majid Valipour <majidvp@chromium.org> > > Reviewed-by: Robert Flack <flackr@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#574781} > > TBR=flackr@chromium.org,majidvp@chromium.org,sunyunjia@chromium.org > > Change-Id: I55c77c05c0381c8ac638bd106d2d18b1b4332745 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 862406 , 862571 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel > Reviewed-on: https://chromium-review.googlesource.com/1136171 > Reviewed-by: Taiju Tsuiki <tzik@chromium.org> > Commit-Queue: Taiju Tsuiki <tzik@chromium.org> > Cr-Commit-Position: refs/heads/master@{#574825} TBR=flackr@chromium.org,majidvp@chromium.org,sunyunjia@chromium.org,tzik@chromium.org Change-Id: I4a9480b68c15e0dfcfd13df6ed6c0e6b8ab5a8e3 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 862406 , 862571 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Reviewed-on: https://chromium-review.googlesource.com/1136671 Reviewed-by: Sandra Sun <sunyunjia@chromium.org> Commit-Queue: Sandra Sun <sunyunjia@chromium.org> Cr-Commit-Position: refs/heads/master@{#574926} [modify] https://crrev.com/043b97cee41154a32d6c1cd8965075813a9ba672/cc/input/scroll_snap_data.cc [modify] https://crrev.com/043b97cee41154a32d6c1cd8965075813a9ba672/third_party/WebKit/LayoutTests/VirtualTestSuites [modify] https://crrev.com/043b97cee41154a32d6c1cd8965075813a9ba672/third_party/WebKit/LayoutTests/external/wpt/css/css-scroll-snap/snap-at-user-scroll-end-manual.html [modify] https://crrev.com/043b97cee41154a32d6c1cd8965075813a9ba672/third_party/WebKit/LayoutTests/external/wpt_automation/css/css-scroll-snap/snap-at-user-scroll-end-manual-automation.js [modify] https://crrev.com/043b97cee41154a32d6c1cd8965075813a9ba672/third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_common_input.js [add] https://crrev.com/043b97cee41154a32d6c1cd8965075813a9ba672/third_party/WebKit/LayoutTests/virtual/threaded/external/wpt/css/css-scroll-snap/README.txt [modify] https://crrev.com/043b97cee41154a32d6c1cd8965075813a9ba672/third_party/blink/renderer/core/input/scroll_manager.cc
,
Jul 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/004406cfc68fcdda58ed1a0f771f237df7837de3 commit 004406cfc68fcdda58ed1a0f771f237df7837de3 Author: Sandra Sun <sunyunjia@chromium.org> Date: Fri Jul 13 15:55:34 2018 Revert "Reland "Correctly initialize and test SnapContainerData in cc."" This reverts commit 043b97cee41154a32d6c1cd8965075813a9ba672. Reason for revert: <INSERT REASONING HERE> Original change's description: > Reland "Correctly initialize and test SnapContainerData in cc." > > This reverts commit 0fc70abb02f52598f0f98eafd7001490545bc0c9. > > Reason for revert: <INSERT REASONING HERE> > > Original change's description: > > Revert "Correctly initialize and test SnapContainerData in cc." > > > > This reverts commit 37061f875b167ab82f1a0ddca84b510b7c6802dc. > > > > Reason for revert: > > The layout tests added by this CL is failing on the CL: > > https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Win10/37804 > > > > Original change's description: > > > Correctly initialize and test SnapContainerData in cc. > > > > > > This patch initializes the rect_ field in SnapContainerData in copy > > > constructor and move constructor. Fixing a snapping issue in the > > > composited pages. > > > > > > We also adds external/wpt/css/css-scroll-snap/ to the virtual/threaded > > > test suite so that they are tested with the composited cases. > > > > > > snap-at-user-scroll-end-manual-automation.js calls > > > mouseClickInTarget() of pointerevent_common_input.js. That method > > > invokes programmatic scrolls. The test was written before programmatic > > > scroll snapping was implemented so it worked at that time. However, > > > with programmatic scroll snapping implemented, it will always snap > > > in mouseClickIntarget() and cannot test whether the user scroll snaps. > > > This patch adds a parameter shouldScrollToTarget to mouseClickInTarget > > > to avoid invoking programmatic scrolls in the test. > > > > > > This patch also checks nullptr for layout_box in > > > ScrollManager::SnapAtGestureScrollEnd() to fix a crash. > > > > > > Bug: 862406 , 862571 > > > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel > > > Change-Id: I6a53818cf74421a4100ad6f908158abf302a5b8e > > > Reviewed-on: https://chromium-review.googlesource.com/1132386 > > > Commit-Queue: Sandra Sun <sunyunjia@chromium.org> > > > Reviewed-by: Majid Valipour <majidvp@chromium.org> > > > Reviewed-by: Robert Flack <flackr@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#574781} > > > > TBR=flackr@chromium.org,majidvp@chromium.org,sunyunjia@chromium.org > > > > Change-Id: I55c77c05c0381c8ac638bd106d2d18b1b4332745 > > No-Presubmit: true > > No-Tree-Checks: true > > No-Try: true > > Bug: 862406 , 862571 > > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel > > Reviewed-on: https://chromium-review.googlesource.com/1136171 > > Reviewed-by: Taiju Tsuiki <tzik@chromium.org> > > Commit-Queue: Taiju Tsuiki <tzik@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#574825} > > TBR=flackr@chromium.org,majidvp@chromium.org,sunyunjia@chromium.org,tzik@chromium.org > > Change-Id: I4a9480b68c15e0dfcfd13df6ed6c0e6b8ab5a8e3 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 862406 , 862571 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel > Reviewed-on: https://chromium-review.googlesource.com/1136671 > Reviewed-by: Sandra Sun <sunyunjia@chromium.org> > Commit-Queue: Sandra Sun <sunyunjia@chromium.org> > Cr-Commit-Position: refs/heads/master@{#574926} TBR=flackr@chromium.org,majidvp@chromium.org,sunyunjia@chromium.org,tzik@chromium.org Change-Id: If9d1e6f73393a7dddc9f8f418606946f070b45be No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 862406 , 862571 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Reviewed-on: https://chromium-review.googlesource.com/1136612 Reviewed-by: Sandra Sun <sunyunjia@chromium.org> Commit-Queue: Sandra Sun <sunyunjia@chromium.org> Cr-Commit-Position: refs/heads/master@{#574931} [modify] https://crrev.com/004406cfc68fcdda58ed1a0f771f237df7837de3/cc/input/scroll_snap_data.cc [modify] https://crrev.com/004406cfc68fcdda58ed1a0f771f237df7837de3/third_party/WebKit/LayoutTests/VirtualTestSuites [modify] https://crrev.com/004406cfc68fcdda58ed1a0f771f237df7837de3/third_party/WebKit/LayoutTests/external/wpt/css/css-scroll-snap/snap-at-user-scroll-end-manual.html [modify] https://crrev.com/004406cfc68fcdda58ed1a0f771f237df7837de3/third_party/WebKit/LayoutTests/external/wpt_automation/css/css-scroll-snap/snap-at-user-scroll-end-manual-automation.js [modify] https://crrev.com/004406cfc68fcdda58ed1a0f771f237df7837de3/third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_common_input.js [delete] https://crrev.com/08a9265741894995d0506d08c8fbee9e26d69b21/third_party/WebKit/LayoutTests/virtual/threaded/external/wpt/css/css-scroll-snap/README.txt [modify] https://crrev.com/004406cfc68fcdda58ed1a0f771f237df7837de3/third_party/blink/renderer/core/input/scroll_manager.cc
,
Jul 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4cf0c38749533c7fffa5eb02b3b0a263032d75c2 commit 4cf0c38749533c7fffa5eb02b3b0a263032d75c2 Author: Sandra Sun <sunyunjia@chromium.org> Date: Fri Jul 13 18:15:50 2018 Reland "Correctly initialize and test SnapContainerData in cc." This reverts commit 0fc70abb02f52598f0f98eafd7001490545bc0c9. This patch increases the no-animation wait time to reduce flakiness. Reason for revert: <INSERT REASONING HERE> Original change's description: > Revert "Correctly initialize and test SnapContainerData in cc." > > This reverts commit 37061f875b167ab82f1a0ddca84b510b7c6802dc. > > Reason for revert: > The layout tests added by this CL is failing on the CL: > https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Win10/37804 > > Original change's description: > > Correctly initialize and test SnapContainerData in cc. > > > > This patch initializes the rect_ field in SnapContainerData in copy > > constructor and move constructor. Fixing a snapping issue in the > > composited pages. > > > > We also adds external/wpt/css/css-scroll-snap/ to the virtual/threaded > > test suite so that they are tested with the composited cases. > > > > snap-at-user-scroll-end-manual-automation.js calls > > mouseClickInTarget() of pointerevent_common_input.js. That method > > invokes programmatic scrolls. The test was written before programmatic > > scroll snapping was implemented so it worked at that time. However, > > with programmatic scroll snapping implemented, it will always snap > > in mouseClickIntarget() and cannot test whether the user scroll snaps. > > This patch adds a parameter shouldScrollToTarget to mouseClickInTarget > > to avoid invoking programmatic scrolls in the test. > > > > This patch also checks nullptr for layout_box in > > ScrollManager::SnapAtGestureScrollEnd() to fix a crash. > > > > Bug: 862406 , 862571 > > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel > > Change-Id: I6a53818cf74421a4100ad6f908158abf302a5b8e > > Reviewed-on: https://chromium-review.googlesource.com/1132386 > > Commit-Queue: Sandra Sun <sunyunjia@chromium.org> > > Reviewed-by: Majid Valipour <majidvp@chromium.org> > > Reviewed-by: Robert Flack <flackr@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#574781} > > TBR=flackr@chromium.org,majidvp@chromium.org,sunyunjia@chromium.org > > Change-Id: I55c77c05c0381c8ac638bd106d2d18b1b4332745 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 862406 , 862571 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel > Reviewed-on: https://chromium-review.googlesource.com/1136171 > Reviewed-by: Taiju Tsuiki <tzik@chromium.org> > Commit-Queue: Taiju Tsuiki <tzik@chromium.org> > Cr-Commit-Position: refs/heads/master@{#574825} TBR=flackr@chromium.org,majidvp@chromium.org,sunyunjia@chromium.org,tzik@chromium.org Change-Id: I85550f6f01b2466d8ceb03e44c15253079a5059c No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 862406 , 862571 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Reviewed-on: https://chromium-review.googlesource.com/1136613 Commit-Queue: Sandra Sun <sunyunjia@chromium.org> Reviewed-by: Sandra Sun <sunyunjia@chromium.org> Cr-Commit-Position: refs/heads/master@{#574983} [modify] https://crrev.com/4cf0c38749533c7fffa5eb02b3b0a263032d75c2/cc/input/scroll_snap_data.cc [modify] https://crrev.com/4cf0c38749533c7fffa5eb02b3b0a263032d75c2/third_party/WebKit/LayoutTests/VirtualTestSuites [modify] https://crrev.com/4cf0c38749533c7fffa5eb02b3b0a263032d75c2/third_party/WebKit/LayoutTests/external/wpt/css/css-scroll-snap/snap-at-user-scroll-end-manual.html [modify] https://crrev.com/4cf0c38749533c7fffa5eb02b3b0a263032d75c2/third_party/WebKit/LayoutTests/external/wpt_automation/css/css-scroll-snap/snap-at-user-scroll-end-manual-automation.js [modify] https://crrev.com/4cf0c38749533c7fffa5eb02b3b0a263032d75c2/third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_common_input.js [add] https://crrev.com/4cf0c38749533c7fffa5eb02b3b0a263032d75c2/third_party/WebKit/LayoutTests/virtual/threaded/external/wpt/css/css-scroll-snap/README.txt [modify] https://crrev.com/4cf0c38749533c7fffa5eb02b3b0a263032d75c2/third_party/blink/renderer/core/input/scroll_manager.cc
,
Aug 17
Looks like the last patch here stuck? Can we close this or is there something left to do?
,
Aug 17
|
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Jul 13