|
|
DescriptionUpstream position: sticky tests for script insertion and layout to WPT
BUG=699244
Review-Url: https://codereview.chromium.org/2926493002
Cr-Commit-Position: refs/heads/master@{#481536}
Committed: https://chromium.googlesource.com/chromium/src/+/a2d610c5c1b83d70c9eac38f45e683a88c9cd9f8
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address reviwer comments #Patch Set 3 : Rebase #Patch Set 4 : Rebase #
Messages
Total messages: 28 (20 generated)
The CQ bit was checked by smcgruer@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: This issue passed the CQ dry run.
smcgruer@chromium.org changed reviewers: + flackr@chromium.org, yigu@chromium.org
lgtm with nits https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-get-bounding-client-rect.html (right): https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-get-bounding-client-rect.html:80: <div id="spacer3" class="spacer"></div> nits: Unused id name.
https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-get-bounding-client-rect.html (right): https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-get-bounding-client-rect.html:69: scroller.insertBefore(sticky, document.getElementById('spacer2')); perhaps even better than giving it an id, document.querySelector('#scroller2 > .spacer') or scroller.querySelector('.spacer') https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-get-bounding-client-rect.html:90: scroller.append(document.createElement('div')); Wouldn't it be more exciting to append the div before the sticky element and give it some height so that you have to re-establish the sticky box rect? https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-offset-top-left.html (right): https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-offset-top-left.html:75: }, 'offsetTop/offsetLeft should be correct for sticky after script-caused layout'); These seem like they should have all of the same use cases as getBoundingClientRect. We might be able to share the code by putting it in a script file included by both tests and passing in a different way to get the element location.
https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-get-bounding-client-rect.html (right): https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-get-bounding-client-rect.html:69: scroller.insertBefore(sticky, document.getElementById('spacer2')); On 2017/06/07 18:32:56, flackr wrote: > perhaps even better than giving it an id, document.querySelector('#scroller2 > > .spacer') or scroller.querySelector('.spacer') Done. https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-get-bounding-client-rect.html:80: <div id="spacer3" class="spacer"></div> On 2017/06/06 21:32:01, yigu wrote: > nits: Unused id name. Done. https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-get-bounding-client-rect.html:90: scroller.append(document.createElement('div')); On 2017/06/07 18:32:56, flackr wrote: > Wouldn't it be more exciting to append the div before the sticky element and > give it some height so that you have to re-establish the sticky box rect? Done. https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-offset-top-left.html (right): https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-offset-top-left.html:75: }, 'offsetTop/offsetLeft should be correct for sticky after script-caused layout'); On 2017/06/07 18:32:56, flackr wrote: > These seem like they should have all of the same use cases as > getBoundingClientRect. We might be able to share the code by putting it in a > script file included by both tests and passing in a different way to get the > element location. I'm not convinced by the code savings here. How strongly do you feel about this?
The CQ bit was checked by smcgruer@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 checked by smcgruer@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by smcgruer@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: This issue passed the CQ dry run.
LGTM, I could go either way on whether the test cases are shared. https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-offset-top-left.html (right): https://codereview.chromium.org/2926493002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-offset-top-left.html:75: }, 'offsetTop/offsetLeft should be correct for sticky after script-caused layout'); On 2017/06/07 20:57:32, smcgruer wrote: > On 2017/06/07 18:32:56, flackr wrote: > > These seem like they should have all of the same use cases as > > getBoundingClientRect. We might be able to share the code by putting it in a > > script file included by both tests and passing in a different way to get the > > element location. > > I'm not convinced by the code savings here. How strongly do you feel about this? It's not code savings but completeness - these should technically be testing the same cases but they have already diverged :-). That being said I don't feel very strongly about this.
The CQ bit was checked by smcgruer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yigu@chromium.org, flackr@chromium.org Link to the patchset: https://codereview.chromium.org/2926493002/#ps60001 (title: "Rebase")
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": 1498139538358320, "parent_rev": "e8961f25e7bdf5bb821821b227924818756c7373", "commit_rev": "a2d610c5c1b83d70c9eac38f45e683a88c9cd9f8"}
Message was sent while issue was closed.
Description was changed from ========== Upstream position: sticky tests for script insertion and layout to WPT BUG=699244 ========== to ========== Upstream position: sticky tests for script insertion and layout to WPT BUG=699244 Review-Url: https://codereview.chromium.org/2926493002 Cr-Commit-Position: refs/heads/master@{#481536} Committed: https://chromium.googlesource.com/chromium/src/+/a2d610c5c1b83d70c9eac38f45e6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a2d610c5c1b83d70c9eac38f45e6...
Message was sent while issue was closed.
waxmigs2902@gmail.com changed reviewers: + waxmigs2902@gmail.com
Message was sent while issue was closed.
burden |