|
|
Created:
4 years, 9 months ago by Mr. Kevin Modified:
4 years, 8 months ago CC:
chromium-reviews, shans, rjwright, blink-reviews-style_chromium.org, blink-reviews-animation_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptiongetComputedStyle now returns used values for top, left, bottom, right
getComputedStyle now returns used values for the top, left, bottom, and
right properties of positioned elements, instead of returning computed
values, if the display property is not None.
R=mstensho@opera.com
BUG=589347
Committed: https://crrev.com/6a2ed97b65b691947eeffbf44ff48a8af2ddff3a
Cr-Commit-Position: refs/heads/master@{#385493}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Incorporated review comments #
Total comments: 8
Patch Set 3 : Incorporated review comments #
Total comments: 1
Patch Set 4 : Fixed dialog layout tests #Messages
Total messages: 46 (16 generated)
On 2016/03/25 01:43:32, khart wrote: This is my first patch. Please run try for me.
Description was changed from ========== fix getComputedStyle positioned element values getComputedStyle now returns used values for the top, left, bottom, and right properties of positioned elements, instead of returning computed values, if the display property is not None. R=mstensho@opera.com BUG=chromium:589347 ========== to ========== fix getComputedStyle positioned element values getComputedStyle now returns used values for the top, left, bottom, and right properties of positioned elements, instead of returning computed values, if the display property is not None. R=mstensho@opera.com BUG=chromium:589347 ==========
khart@codeaurora.org changed reviewers: + rune@opera.com
The CQ bit was checked by timloh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826423003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826423003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: The author khart@codeaurora.org has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by rune@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826423003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826423003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: The author khart@codeaurora.org has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
It seems that you still need to go through some paperwork to become an official contributor. Please visit https://cla.developers.google.com https://codereview.chromium.org/1826423003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/1826423003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:195: if (style.display() != NONE) { Can just check if you have a layoutObject instead of checking 'display'. https://codereview.chromium.org/1826423003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:199: // If e.g. left is auto and right is not auto, then left's computed value is negative right(). Should remove "()" from "right()". https://codereview.chromium.org/1826423003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:203: } No need for curly braces here. https://codereview.chromium.org/1826423003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:204: opposite *= -1.f; Why not "opposite = -opposite"? Oh, right, Length doesn't provide operator-(). We could add it, but I wonder if we can do without this negation, by just modifying the code below to eliminate the need for it. Performing arithmetic on Length variables seems like a very uncommon thing to do. E.g. zoomAdjustedPixelValueForLength(opposite, style) could be written as zoomAdjustedPixelValue(-opposite.pixels(), style) https://codereview.chromium.org/1826423003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:212: return zoomAdjustedPixelValueForLength(opposite, style); Note: We don't take care of over-constrained situations, where both |left| and |right| or both |top| and |bottom| are non-auto [1][2], but this has always been missing in this function, so we can worry about that later, I suppose. Maybe file a CL at some point that fixes this completely for the entire function. [1] Relpos: https://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#relative-positioning [2] Abspos: https://www.w3.org/TR/2011/REC-CSS2-20110607/visudet.html#abs-non-replaced-width https://codereview.chromium.org/1826423003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:226: LayoutUnit left = locationOffset.width() - container->borderLeft() - layoutBox->marginLeft(); We subtract layoutBox->marginLeft() here, which is correct for case CSSPropertyLeft, but if it's CSSPropertyRight, we have to compensate for that again further below, which just seems convoluted. I think the code would look cleaner if we didn't declare |left| and |top|, and instead performed the necessary calculations for each of the 4 cases. Might look nice if you defined locationOffset (or whatever you prefer to call it instead) as layoutBox->locationOffset() - LayoutSize(container->clientLeft(), container->clientTop()), maybe? (Yes, better use clientLeft() and clientTop() instead of borderLeft() and borderTop() here, so that left-handed scrollbars get handled correctly.) https://codereview.chromium.org/1826423003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:247: return nullptr; You have handled all the possible cases, so better just do an ASSERT_NOT_REACHED() here.
Thanks for the great review. I have incorporated the changes and signed the CLA, and the new patch is almost ready to go. I should be able to push it tomorrow. I ran into an issue with LayoutTests/fast/dom/HTMLDialogElement/abspos-dialog-layout.html. When it hits my code, container->clientHeight() is 600 and locationOffset.top() is 780. This results in a negative bottom position, which confused me. I've convinced myself this is correct, since the container is the ICB, which has the same dimensions as the viewport, and the absolutely positioned dialog is initially not visible. When the new patch arrives, please look carefully at my changes to the dialog layout tests.
The CQ bit was checked by khart@codeaurora.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826423003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826423003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Please run try.
On 2016/04/01 17:22:30, khart wrote: > Please run try. Triggered a linux bot for you. Didn't try a full dry run as that failed last time I tried.
You also need to add yourself to the AUTHORS file (in the Chromium root). https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/animations/change-keyframes.html (left): https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes.html:56: // The left property should reset to auto and top should be animating. No longer "auto", so you should update the comment too. https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/HTMLDialogElement/abspos-dialog-layout.html (right): https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/HTMLDialogElement/abspos-dialog-layout.html:41: shouldBeEqualToString('window.getComputedStyle(dialog).top', '790px'); Yes, I think this is reasonable and correct behavior. The dialog is centered. If the viewport (and initial containing block) height is 600px, and line height is 20px, we end up with a dialog whose top is 290px below the top of the initial containing block and whose bottom is 290px above the bottom of the ICB. However, we are scrolled by 0,500, so when the dialog is opened, its position is adjusted accordingly. The ICB is always at scroll position 0,0, no matter what the actual scroll position is. Since Blink is the only engine that supports DIALOG, we have nobody to compare against, though. https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (left): https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:189: if (offset.isAuto()) { "if (offset.isAuto() && layoutObject)", to reduce indentation and lines of code? :) https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:207: toLayoutBox(layoutObject)->containingBlockLogicalWidthForContent() : Using logical width and height to resolve physical left/top/right/bottom is actually wrong (if writing mode is vertical), but this is what the percentage handling code above already does too, so, fine, better be consistent. :)
I believe I am covered by *@codearuora.org in the AUTHORS file. https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/animations/change-keyframes.html (left): https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes.html:56: // The left property should reset to auto and top should be animating. On 2016/04/04 09:13:26, mstensho wrote: > No longer "auto", so you should update the comment too. Done. https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/HTMLDialogElement/abspos-dialog-layout.html (right): https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/HTMLDialogElement/abspos-dialog-layout.html:41: shouldBeEqualToString('window.getComputedStyle(dialog).top', '790px'); On 2016/04/04 09:13:26, mstensho wrote: > Yes, I think this is reasonable and correct behavior. The dialog is centered. If > the viewport (and initial containing block) height is 600px, and line height is > 20px, we end up with a dialog whose top is 290px below the top of the initial > containing block and whose bottom is 290px above the bottom of the ICB. However, > we are scrolled by 0,500, so when the dialog is opened, its position is adjusted > accordingly. The ICB is always at scroll position 0,0, no matter what the actual > scroll position is. > > Since Blink is the only engine that supports DIALOG, we have nobody to compare > against, though. Acknowledged. https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (left): https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:189: if (offset.isAuto()) { On 2016/04/04 09:13:26, mstensho wrote: > "if (offset.isAuto() && layoutObject)", to reduce indentation and lines of code? > :) It falls through at the end to handle isAuto() && !layoutObject, but it's clearer and has less indentation to separate that condition out, so done. https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:207: toLayoutBox(layoutObject)->containingBlockLogicalWidthForContent() : On 2016/04/04 09:13:26, mstensho wrote: > Using logical width and height to resolve physical left/top/right/bottom is > actually wrong (if writing mode is vertical), but this is what the percentage > handling code above already does too, so, fine, better be consistent. :) Acknowledged.
No new patch uploaded? :) Also, could you fix the description a little? I think I'd like the first line to mention top, right, bottom and left, without turning it into an insanely long sentence. Also, the "chromium:" prefix after BUG= is unknown at least to me. Maybe you could just remove it?
Description was changed from ========== fix getComputedStyle positioned element values getComputedStyle now returns used values for the top, left, bottom, and right properties of positioned elements, instead of returning computed values, if the display property is not None. R=mstensho@opera.com BUG=chromium:589347 ========== to ========== getComputedStyle now returns used values for top, left, bottom, right getComputedStyle now returns used values for the top, left, bottom, and right properties of positioned elements, instead of returning computed values, if the display property is not None. R=mstensho@opera.com BUG=589347 ==========
On 2016/04/04 18:34:03, mstensho wrote: > No new patch uploaded? :) > > Also, could you fix the description a little? I think I'd like the first line to > mention top, right, bottom and left, without turning it into an insanely long > sentence. > > Also, the "chromium:" prefix after BUG= is unknown at least to me. Maybe you > could just remove it? Sorry, had a delay on this end. Just as well, since now I've (hopefully) improved the description and gotten rid of the chromium: prefix. Regarding over-constrained position properties, the spec seems pretty clear, but Firefox/Windows and IE also return the declared values for over-constrained positioned elements, as far as I can tell. All three browsers display over-constrained elements correctly, so the "actual" values must be correct. What I'm not sure of is whether the resolution of over-constrained values happens between "computed" and "used", or between "used" and "actual" in the sequence of value processing. Should I go ahead and raise a CR anyway?
lgtm. And welcome aboard as a contributor! :) Yes, I couldn't find any other browser that handled over-constrained situations correctly either, when it comes to getComputedStyle() for these properties. Resolution of over-constrained situations should take place between "computed" and "used". It's definitely worth reporting a bug (if you cannot find an existing report). https://codereview.chromium.org/1826423003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/1826423003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:248: return cssValuePool().createIdentifierValue(CSSValueAuto); Sorry! Didn't realize that this one was there to handle !layoutObject or static positioned. The code was good the way it was. It had exactly as many indentation levels as it needed. :) Feel free to revert to how it was, or just ignore this comment. I'm fine either way.
On 2016/04/04 22:19:20, mstensho wrote: > lgtm. And welcome aboard as a contributor! :) Thanks! > Yes, I couldn't find any other browser that handled over-constrained situations > correctly either, when it comes to getComputedStyle() for these properties. > > Resolution of over-constrained situations should take place between "computed" > and "used". It's definitely worth reporting a bug (if you cannot find an > existing report). OK, will do. > https://codereview.chromium.org/1826423003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp > (right): > > https://codereview.chromium.org/1826423003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:248: return > cssValuePool().createIdentifierValue(CSSValueAuto); > Sorry! Didn't realize that this one was there to handle !layoutObject or static > positioned. The code was good the way it was. It had exactly as many indentation > levels as it needed. :) > > Feel free to revert to how it was, or just ignore this comment. I'm fine either > way. I'm happy with the way it is.
The CQ bit was checked by khart@codeaurora.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826423003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826423003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/04/05 00:26:33, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) Looks like the dialog element height is 18 on the try system instead of 20, as it was on my system. Dialog height is "auto" (unless it changed upstream, I'll check that) Does this mean I have a configuration problem on my system, or should I set the dialog height, or something else?
On 2016/04/05 01:19:44, khart wrote: > On 2016/04/05 00:26:33, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > Looks like the dialog element height is 18 on the try system instead of 20, as > it was on my system. Dialog height is "auto" (unless it changed upstream, I'll > check that) Does this mean I have a configuration problem on my system, or > should I set the dialog height, or something else? Try setting line-height:20px on the DIALOG element?
The CQ bit was checked by khart@codeaurora.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826423003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826423003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/05 07:16:44, mstensho wrote: > On 2016/04/05 01:19:44, khart wrote: > > On 2016/04/05 00:26:33, commit-bot: I haz the power wrote: > > > Dry run: Try jobs failed on following builders: > > > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > > > Looks like the dialog element height is 18 on the try system instead of 20, as > > it was on my system. Dialog height is "auto" (unless it changed upstream, > I'll > > check that) Does this mean I have a configuration problem on my system, or > > should I set the dialog height, or something else? > > Try setting line-height:20px on the DIALOG element? Passed dry run
On 2016/04/06 00:08:07, khart wrote: > On 2016/04/05 07:16:44, mstensho wrote: > > On 2016/04/05 01:19:44, khart wrote: > > > On 2016/04/05 00:26:33, commit-bot: I haz the power wrote: > > > > Dry run: Try jobs failed on following builders: > > > > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > > > > > Looks like the dialog element height is 18 on the try system instead of 20, > as > > > it was on my system. Dialog height is "auto" (unless it changed upstream, > > I'll > > > check that) Does this mean I have a configuration problem on my system, or > > > should I set the dialog height, or something else? > > > > Try setting line-height:20px on the DIALOG element? > > Passed dry run Just check the "Commit" checkbox when you're ready. :)
The CQ bit was checked by khart@codeaurora.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com Link to the patchset: https://codereview.chromium.org/1826423003/#ps60001 (title: "Fixed dialog layout tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826423003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826423003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== getComputedStyle now returns used values for top, left, bottom, right getComputedStyle now returns used values for the top, left, bottom, and right properties of positioned elements, instead of returning computed values, if the display property is not None. R=mstensho@opera.com BUG=589347 ========== to ========== getComputedStyle now returns used values for top, left, bottom, right getComputedStyle now returns used values for the top, left, bottom, and right properties of positioned elements, instead of returning computed values, if the display property is not None. R=mstensho@opera.com BUG=589347 Committed: https://crrev.com/6a2ed97b65b691947eeffbf44ff48a8af2ddff3a Cr-Commit-Position: refs/heads/master@{#385493} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6a2ed97b65b691947eeffbf44ff48a8af2ddff3a Cr-Commit-Position: refs/heads/master@{#385493}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1888533002/ by wangxianzhu@chromium.org. The reason for reverting is: Caused crbug.com/602934. BUG=602934,589347. |