Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(353)

Issue 1826423003: fix getComputedStyle positioned element values (Closed)

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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+354 lines, -29 lines) Patch
M third_party/WebKit/LayoutTests/animations/animation-properties-in-keyframe-are-ignored.html View 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/animations/animation-properties-in-keyframe-are-ignored-expected.txt View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/animations/change-keyframes.html View 1 2 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/animations/play-state.html View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/getComputedStyle-resolved-values.html View 5 chunks +45 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/getComputedStyle-resolved-values-expected.txt View 21 chunks +220 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/hover-affects-child.html View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/dom/HTMLDialogElement/abspos-dialog-layout.html View 1 2 3 1 chunk +6 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/dom/HTMLDialogElement/abspos-dialog-layout-expected.txt View 1 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/dom/HTMLDialogElement/fixpos-dialog-layout.html View 1 2 3 1 chunk +6 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/dom/HTMLDialogElement/fixpos-dialog-layout-expected.txt View 1 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp View 1 2 2 chunks +60 lines, -6 lines 0 comments Download

Messages

Total messages: 46 (16 generated)
Mr. Kevin
4 years, 9 months ago (2016-03-25 01:43:32 UTC) #1
Mr. Kevin
On 2016/03/25 01:43:32, khart wrote: This is my first patch. Please run try for me.
4 years, 9 months ago (2016-03-25 01:49:39 UTC) #2
commit-bot: I haz the power
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
4 years, 8 months ago (2016-03-29 00:05:33 UTC) #6
commit-bot: I haz the power
Dry run: The author khart@codeaurora.org has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com ...
4 years, 8 months ago (2016-03-29 00:05:35 UTC) #8
commit-bot: I haz the power
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
4 years, 8 months ago (2016-03-29 10:56:56 UTC) #10
commit-bot: I haz the power
Dry run: The author khart@codeaurora.org has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com ...
4 years, 8 months ago (2016-03-29 10:56:58 UTC) #12
mstensho (USE GERRIT)
It seems that you still need to go through some paperwork to become an official ...
4 years, 8 months ago (2016-03-29 13:13:26 UTC) #13
Mr. Kevin
Thanks for the great review. I have incorporated the changes and signed the CLA, and ...
4 years, 8 months ago (2016-03-31 00:47:48 UTC) #14
commit-bot: I haz the power
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
4 years, 8 months ago (2016-04-01 00:20:51 UTC) #16
commit-bot: I haz the power
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started ...
4 years, 8 months ago (2016-04-01 00:20:54 UTC) #18
Mr. Kevin
Please run try.
4 years, 8 months ago (2016-04-01 17:22:30 UTC) #19
rune
On 2016/04/01 17:22:30, khart wrote: > Please run try. Triggered a linux bot for you. ...
4 years, 8 months ago (2016-04-02 00:44:04 UTC) #20
mstensho (USE GERRIT)
You also need to add yourself to the AUTHORS file (in the Chromium root). https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/LayoutTests/animations/change-keyframes.html ...
4 years, 8 months ago (2016-04-04 09:13:26 UTC) #21
Mr. Kevin
I believe I am covered by *@codearuora.org in the AUTHORS file. https://codereview.chromium.org/1826423003/diff/20001/third_party/WebKit/LayoutTests/animations/change-keyframes.html File third_party/WebKit/LayoutTests/animations/change-keyframes.html (left): ...
4 years, 8 months ago (2016-04-04 17:35:22 UTC) #22
mstensho (USE GERRIT)
No new patch uploaded? :) Also, could you fix the description a little? I think ...
4 years, 8 months ago (2016-04-04 18:34:03 UTC) #23
Mr. Kevin
On 2016/04/04 18:34:03, mstensho wrote: > No new patch uploaded? :) > > Also, could ...
4 years, 8 months ago (2016-04-04 21:44:15 UTC) #25
mstensho (USE GERRIT)
lgtm. And welcome aboard as a contributor! :) Yes, I couldn't find any other browser ...
4 years, 8 months ago (2016-04-04 22:19:20 UTC) #26
Mr. Kevin
On 2016/04/04 22:19:20, mstensho wrote: > lgtm. And welcome aboard as a contributor! :) Thanks! ...
4 years, 8 months ago (2016-04-04 22:53:56 UTC) #27
commit-bot: I haz the power
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
4 years, 8 months ago (2016-04-04 22:58:30 UTC) #29
commit-bot: I haz the power
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_ng/builds/199234)
4 years, 8 months ago (2016-04-05 00:26:33 UTC) #31
Mr. Kevin
On 2016/04/05 00:26:33, commit-bot: I haz the power wrote: > Dry run: Try jobs failed ...
4 years, 8 months ago (2016-04-05 01:19:44 UTC) #32
mstensho (USE GERRIT)
On 2016/04/05 01:19:44, khart wrote: > On 2016/04/05 00:26:33, commit-bot: I haz the power wrote: ...
4 years, 8 months ago (2016-04-05 07:16:44 UTC) #33
commit-bot: I haz the power
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
4 years, 8 months ago (2016-04-05 17:23:19 UTC) #35
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 8 months ago (2016-04-05 18:39:09 UTC) #37
Mr. Kevin
On 2016/04/05 07:16:44, mstensho wrote: > On 2016/04/05 01:19:44, khart wrote: > > On 2016/04/05 ...
4 years, 8 months ago (2016-04-06 00:08:07 UTC) #38
mstensho (USE GERRIT)
On 2016/04/06 00:08:07, khart wrote: > On 2016/04/05 07:16:44, mstensho wrote: > > On 2016/04/05 ...
4 years, 8 months ago (2016-04-06 07:52:56 UTC) #39
commit-bot: I haz the power
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
4 years, 8 months ago (2016-04-06 17:19:14 UTC) #42
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 8 months ago (2016-04-06 17:25:18 UTC) #43
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/6a2ed97b65b691947eeffbf44ff48a8af2ddff3a Cr-Commit-Position: refs/heads/master@{#385493}
4 years, 8 months ago (2016-04-06 17:27:28 UTC) #45
Xianzhu
4 years, 8 months ago (2016-04-13 18:12:13 UTC) #46
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.

Powered by Google App Engine
This is Rietveld 408576698