Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2907243002:
Sticky position should be observable by getBoundingClientRect (Closed)
DescriptionSticky position should be observable by getBoundingClientRect.
BUG=699244
Review-Url: https://codereview.chromium.org/2907243002
Cr-Commit-Position: refs/heads/master@{#476392}
Committed: https://chromium.googlesource.com/chromium/src/+/9bf01e4dcb9fce62221d4e3ac0c959f2f5f26f84
Patch Set 1 #Patch Set 2 : nit #
Total comments: 2
Patch Set 3 : Use js test instead of ref test #
Total comments: 6
Patch Set 4 : nit #Messages
Total messages: 15 (7 generated)
yigu@chromium.org changed reviewers: + flackr@chromium.org
Hi Rob. PTAL. Thanks!
https://codereview.chromium.org/2907243002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html (right): https://codereview.chromium.org/2907243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html:47: sticky1.style.position = 'relative'; I thought the thing we wanted to test was that querying the position from javascript (i.e. offsetTop / getBoundingClientRect etc) was correct after changing the style, which should be done as a JS test rather than a ref test.
Thanks. Test updated. https://codereview.chromium.org/2907243002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html (right): https://codereview.chromium.org/2907243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html:47: sticky1.style.position = 'relative'; On 2017/05/31 14:13:47, flackr wrote: > I thought the thing we wanted to test was that querying the position from > javascript (i.e. offsetTop / getBoundingClientRect etc) was correct after > changing the style, which should be done as a JS test rather than a ref test. Done.
Suggest maybe naming it position-sticky-get-bounding-client-rect.html? BUG=699244 ? Is this replacing an existing layout test or is this a new test? https://codereview.chromium.org/2907243002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html (right): https://codereview.chromium.org/2907243002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html:2: <title>Sticky positioned element should behave correctly when style changes</title> same as below comment. "behave correctly" is a little vague so let's refer to the specific api being tested. https://codereview.chromium.org/2907243002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html:4: <meta name="assert" content="This test checks that sticky positioned element behaves correctly when style changes." /> Same https://codereview.chromium.org/2907243002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html:51: }, 'sticky positioned element should behaves correctly upon style changes.'); nit: title should exactly reflect the test. How about 'sticky position should be observable by getBoundingClientRect.'?
Description was changed from ========== Upstream position: sticky element style change test to WPT BUG= ========== to ========== Upstream position: sticky element style change test to WPT BUG=699244 ==========
Thanks for the suggestion! This is a new test so no existing test needs to be removed. https://codereview.chromium.org/2907243002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html (right): https://codereview.chromium.org/2907243002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html:2: <title>Sticky positioned element should behave correctly when style changes</title> On 2017/05/31 21:10:23, flackr wrote: > same as below comment. "behave correctly" is a little vague so let's refer to > the specific api being tested. Done. https://codereview.chromium.org/2907243002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html:4: <meta name="assert" content="This test checks that sticky positioned element behaves correctly when style changes." /> On 2017/05/31 21:10:23, flackr wrote: > Same Done. https://codereview.chromium.org/2907243002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/css/css-position-3/position-sticky-style-change.html:51: }, 'sticky positioned element should behaves correctly upon style changes.'); On 2017/05/31 21:10:23, flackr wrote: > nit: title should exactly reflect the test. How about 'sticky position should be > observable by getBoundingClientRect.'? Done.
smcgruer@chromium.org changed reviewers: + smcgruer@chromium.org
lgtm
The CQ bit was checked by yigu@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Upstream position: sticky element style change test to WPT BUG=699244 ========== to ========== Sticky position should be observable by getBoundingClientRect. BUG=699244 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496344227343190,
"parent_rev": "cc6214142ae45020926c78c3b1dff4eea7c7eedb", "commit_rev":
"9bf01e4dcb9fce62221d4e3ac0c959f2f5f26f84"}
Message was sent while issue was closed.
Description was changed from ========== Sticky position should be observable by getBoundingClientRect. BUG=699244 ========== to ========== Sticky position should be observable by getBoundingClientRect. BUG=699244 Review-Url: https://codereview.chromium.org/2907243002 Cr-Commit-Position: refs/heads/master@{#476392} Committed: https://chromium.googlesource.com/chromium/src/+/9bf01e4dcb9fce62221d4e3ac0c9... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/9bf01e4dcb9fce62221d4e3ac0c9... |
