Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2870983002:
Update return value of getComputedStyle for sticky elements (Closed)
DescriptionUpdate return value of getComputedStyle for sticky elements
Top/bottom/left/right have special meaning for sticky elements. So when
calling getComputedStyle on sticky elements whose style contains
"top: auto;", we should return "auto" rather than "0px" to eliminate
ambiguity.
Chromium followed the spec which specifies that getComputedStyle should
return used value (0px) for sticky elements with "top: auto;". That is
incorrect. A spec issue has been added:
https://github.com/w3c/csswg-drafts/issues/1346
BUG=703816
TEST=LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html
Review-Url: https://codereview.chromium.org/2870983002
Cr-Commit-Position: refs/heads/master@{#471185}
Committed: https://chromium.googlesource.com/chromium/src/+/c6fcc669f0a2e7d8f9cbf99704a1ca4ca3a8c77f
Patch Set 1 #
Total comments: 6
Patch Set 2 : nit #
Total comments: 2
Patch Set 3 : nit #
Total comments: 4
Patch Set 4 : nit #Patch Set 5 : Update test with testharness.js #
Messages
Total messages: 23 (10 generated)
yigu@chromium.org changed reviewers: + flackr@chromium.org, rbyers@chromium.org
This patch only focuses on position: sticky. "fixed" or "absolute" still return used value which may need to be fixed with further discussion. PTAL. Thanks!
https://codereview.chromium.org/2870983002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html (right): https://codereview.chromium.org/2870983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html:12: top: 2px; nit: use top: 0px to showcase the original issue the bug was about. https://codereview.chromium.org/2870983002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/2870983002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:226: // Therefore "auto" should be returned regardless the opposite. nit: Add a link to the bug for reference. https://codereview.chromium.org/2870983002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:232: if (layout_object->IsInFlowPositioned()) { Sticky is in flow positioned, let's change this condition to not apply to sticky position (i.e. relative position only) and let sticky return auto from line 299 below.
Thanks Rob. PTAL. https://codereview.chromium.org/2870983002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html (right): https://codereview.chromium.org/2870983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html:12: top: 2px; On 2017/05/09 20:26:34, flackr wrote: > nit: use top: 0px to showcase the original issue the bug was about. Done. https://codereview.chromium.org/2870983002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/2870983002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:226: // Therefore "auto" should be returned regardless the opposite. On 2017/05/09 20:26:34, flackr wrote: > nit: Add a link to the bug for reference. Done. https://codereview.chromium.org/2870983002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:232: if (layout_object->IsInFlowPositioned()) { On 2017/05/09 20:26:35, flackr wrote: > Sticky is in flow positioned, let's change this condition to not apply to sticky > position (i.e. relative position only) and let sticky return auto from line 299 > below. Done.
https://codereview.chromium.org/2870983002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/2870983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:229: return CSSIdentifierValue::Create(CSSValueAuto); This extra condition should no longer be necessary given the change on 227 right?
Thanks. Have updated the patch. https://codereview.chromium.org/2870983002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/2870983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:229: return CSSIdentifierValue::Create(CSSValueAuto); On 2017/05/09 20:45:52, flackr wrote: > This extra condition should no longer be necessary given the change on 227 > right? Yes. Done.
LGTM with nits. https://codereview.chromium.org/2870983002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html (right): https://codereview.chromium.org/2870983002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html:21: <body> nit: I think the body tag is not usually included in layout tests. https://codereview.chromium.org/2870983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/2870983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:227: // According to https://crbug.com/703816, "top:auto" and "top:0px" have very nit: Let's just say something to the effect of "Position offsets have special meaning for position sticky so we return auto when offset.isAuto() on a sticky position object: https://crbug.com/703816"
Thanks Rob for the review. https://codereview.chromium.org/2870983002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html (right): https://codereview.chromium.org/2870983002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html:21: <body> On 2017/05/09 21:21:41, flackr wrote: > nit: I think the body tag is not usually included in layout tests. Done. https://codereview.chromium.org/2870983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/2870983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:227: // According to https://crbug.com/703816, "top:auto" and "top:0px" have very On 2017/05/09 21:21:41, flackr wrote: > nit: Let's just say something to the effect of "Position offsets have special > meaning for position sticky so we return auto when offset.isAuto() on a sticky > position object: https://crbug.com/703816%22 Done.
LGTM Please ensure a spec issue is filed for this (describing this as existing behavior) before landing and refer to the spec issue in the commit description. Ideally we'd just add / modify the tests in LayoutTests/external/wpt/css/css-position-3 instead of introducing a chromium-specific test. I guess that test probably can't be landed upstream until the spec issue is resolved. But you should at least be using a WPT-style test so that it can simply be moved from chromium to wpt when the spec change is landed. /cc smcgruer
On 2017/05/09 21:42:39, Rick Byers wrote: > LGTM > > Please ensure a spec issue is filed for this (describing this as existing > behavior) before landing and refer to the spec issue in the commit description. > > Ideally we'd just add / modify the tests in > LayoutTests/external/wpt/css/css-position-3 instead of introducing a > chromium-specific test. I guess that test probably can't be landed upstream > until the spec issue is resolved. But you should at least be using a WPT-style > test so that it can simply be moved from chromium to wpt when the spec change is > landed. /cc smcgruer Yes, would be awesome if the test was WPT-like. At the very least, this can clearly be a js-based test rather than a -expected.txt; all you are verifying is that getComputedStyle(...) returns certain values. I would suggest doing testharness.js tests - http://web-platform-tests.org/writing-tests/testharness.html, which should also work outside of the external/wpt directory for now.
The CQ bit was checked by yigu@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 ========== Update return value of getComputedStyle for sticky elements Top/bottom/left/right have special meaning for sticky elements. So when calling getComputedStyle on elements whose style contains "top: auto;", we should return "auto" rather than "0px" to eliminate ambiguity. BUG=703816 TEST=LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html ========== to ========== Update return value of getComputedStyle for sticky elements Top/bottom/left/right have special meaning for sticky elements. So when calling getComputedStyle on sticky elements whose style contains "top: auto;", we should return "auto" rather than "0px" to eliminate ambiguity. Chromium followed the spec which specifies that getComputedStyle should return used value (0px) for sticky elements with "top: auto;". That is incorrect. A spec issue has been added: https://github.com/w3c/csswg-drafts/issues/1346 BUG=703816 TEST=LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html ==========
On 2017/05/10 13:12:12, smcgruer wrote: > On 2017/05/09 21:42:39, Rick Byers wrote: > > LGTM > > > > Please ensure a spec issue is filed for this (describing this as existing > > behavior) before landing and refer to the spec issue in the commit > description. > > > > Ideally we'd just add / modify the tests in > > LayoutTests/external/wpt/css/css-position-3 instead of introducing a > > chromium-specific test. I guess that test probably can't be landed upstream > > until the spec issue is resolved. But you should at least be using a > WPT-style > > test so that it can simply be moved from chromium to wpt when the spec change > is > > landed. /cc smcgruer > > Yes, would be awesome if the test was WPT-like. At the very least, this can > clearly > be a js-based test rather than a -expected.txt; all you are verifying is that > getComputedStyle(...) returns certain values. > > I would suggest doing testharness.js tests - > http://web-platform-tests.org/writing-tests/testharness.html, > which should also work outside of the external/wpt directory for now. Thanks Stephen. Test updated.
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_...)
Thanks for updating the test, still LGTM.
The CQ bit was checked by yigu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org Link to the patchset: https://codereview.chromium.org/2870983002/#ps80001 (title: "Update test with testharness.js")
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": 80001, "attempt_start_ts": 1494544137161840,
"parent_rev": "b629361e28a37d2d72454e4efcec3d60cb312f2b", "commit_rev":
"c6fcc669f0a2e7d8f9cbf99704a1ca4ca3a8c77f"}
Message was sent while issue was closed.
Description was changed from ========== Update return value of getComputedStyle for sticky elements Top/bottom/left/right have special meaning for sticky elements. So when calling getComputedStyle on sticky elements whose style contains "top: auto;", we should return "auto" rather than "0px" to eliminate ambiguity. Chromium followed the spec which specifies that getComputedStyle should return used value (0px) for sticky elements with "top: auto;". That is incorrect. A spec issue has been added: https://github.com/w3c/csswg-drafts/issues/1346 BUG=703816 TEST=LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html ========== to ========== Update return value of getComputedStyle for sticky elements Top/bottom/left/right have special meaning for sticky elements. So when calling getComputedStyle on sticky elements whose style contains "top: auto;", we should return "auto" rather than "0px" to eliminate ambiguity. Chromium followed the spec which specifies that getComputedStyle should return used value (0px) for sticky elements with "top: auto;". That is incorrect. A spec issue has been added: https://github.com/w3c/csswg-drafts/issues/1346 BUG=703816 TEST=LayoutTests/fast/css/sticky/sticky-top-auto-get-computedstyle.html Review-Url: https://codereview.chromium.org/2870983002 Cr-Commit-Position: refs/heads/master@{#471185} Committed: https://chromium.googlesource.com/chromium/src/+/c6fcc669f0a2e7d8f9cbf99704a1... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/c6fcc669f0a2e7d8f9cbf99704a1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
