Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2816983002:
LayoutTableCell::OffsetFromContainer() should use the flipped offset of its parent. (Closed)
|
Created:
3 years, 8 months ago by yuki.sekiguchi Modified:
3 years, 8 months ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLayoutTableCell::OffsetFromContainer() should use the flipped offset of its parent.
Since LayoutTableCell is relative to LayoutTableSection, it should subtract the
offset of the LayoutTableRow. OffsetFromContainer() is flipped value, but
LayoutTableCell::OffsetFromContainer() uses LocationOffset() which is an unflipped
location. Therefore, the rect of td goes outside of tr.
It should use PhysicalLocationOffset() which returns a flipped location.
BUG=711193
Review-Url: https://codereview.chromium.org/2816983002
Cr-Commit-Position: refs/heads/master@{#467467}
Committed: https://chromium.googlesource.com/chromium/src/+/5cdd84c272ce41e804a9d4447b288f44d748c6af
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixed the review comments #Patch Set 3 : Reformat the test #
Messages
Total messages: 28 (17 generated)
The CQ bit was checked by yuki.sekiguchi@access-company.com 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...
yuki.sekiguchi@access-company.com changed reviewers: + eae@chromium.org
Hi Emil, Could you review my CL? I'm not sure whether you're the right person. Please recommend if someone is better than you.
eae@chromium.org changed reviewers: + dgrogan@chromium.org
dgrogan is the right reviewer for this. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the patch. Don't be discouraged by the number of comments, we are just really picky about style. Is this bug just about the values returned by getBoundingClientRect or is it possible to show the issue visually? If it's possible to show visually, could you update the test to do so? We try to have a page where a viewer can tell at a glance if their is an error because red shows on an error and only green shows on pass. Does that make sense? Of course, if the bug is just about getBoundingClientRect, then disregard the red/green stuff. https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html (right): https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:2: td { padding: 10px } We rarely indent by 1 space, use 2. https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:4: <div style="-webkit-writing-mode: vertical-rl; writing-mode: vertical-rl"> Does safari require the prefixed property? If not, delete it from here. https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:5: <table style="margin: 50px"> Why the margin? https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:42: <div style="padding: 100px"></div> div unneeded? https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:48: var margin = 0; remove margin? https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:77: assert_contains(trRect, spanRect, name); Can you add the 2 clientRect values to the fail message? https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:81: for (var i = 0; i < 5; i++) { Rename i->row and j->column? And, do we need a table with 25 squares? Would reducing to a 2x2 table work just as well? https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-td.html (right): https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-td.html:2: td { padding: 10px } Could you explain more about why we need both tests? If we do need to check the spans, maybe we can merge the two test files so that there are 4 squares in the table and check the tds against the top tr and the spans against the bottom tr? Does that work?
The CQ bit was checked by yuki.sekiguchi@access-company.com 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 yuki.sekiguchi@access-company.com 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...
Hi David, Thank you for reviewing. Fixed your comments. Comments inline: On 2017/04/25 23:23:35, dgrogan wrote: > Thanks for the patch. Don't be discouraged by the number of comments, we are > just really picky about style. No problem. The strict style makes easy to read code :) > Is this bug just about the values returned by getBoundingClientRect or is it > possible to show the issue visually? If it's possible to show visually, could > you update the test to do so? We try to have a page where a viewer can tell at a > glance if their is an error because red shows on an error and only green shows > on pass. Does that make sense? Of course, if the bug is just about > getBoundingClientRect, then disregard the red/green stuff. The function is used by the caret of editing, the theme for search bar and paint invalidation. The caret and the theme checks a rect which is relative to its parent, so this bug won't affect both. The paint invalidation code use this to calculate a bounding box of a layer or a scrollable area. Since a table always contains a td, it won't create a bounding box which is visually invalid. I created test contents for them, but I couldn't reproduce them. > https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html > (right): > > https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:2: > td { padding: 10px } > We rarely indent by 1 space, use 2. Sure. Fixed. > https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:4: > <div style="-webkit-writing-mode: vertical-rl; writing-mode: vertical-rl"> > Does safari require the prefixed property? If not, delete it from here. Yes, it is. Safari doesn't support the non-prefixed writing-mode. > https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:5: > <table style="margin: 50px"> > Why the margin? > Sorry. I forget to remove this. Removed. > https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:42: > <div style="padding: 100px"></div> > div unneeded? Yes. It is unneeded to reproduce the bug. It makes easy to check where the buggy rect is by the inspector, but it isn't needed for auto test. Removed. > https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:48: > var margin = 0; > remove margin? Removed. > https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:77: > assert_contains(trRect, spanRect, name); > Can you add the 2 clientRect values to the fail message? Added. > https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-child.html:81: > for (var i = 0; i < 5; i++) { > Rename i->row and j->column? > And, do we need a table with 25 squares? Would reducing to a 2x2 table work just > as well? Yes. The 2x2 table is enough. > https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-td.html > (right): > > https://codereview.chromium.org/2816983002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/dom/Element/getBoundingClientRect-vertical-td.html:2: > td { padding: 10px } > Could you explain more about why we need both tests? If we do need to check the > spans, maybe we can merge the two test files so that there are 4 squares in the > table and check the tds against the top tr and the spans against the bottom tr? > Does that work? There is no need to check tds as well as children. The content for the children covers my change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dgrogan@chromium.org
lgtm
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
eae, can you review this as OWNER? Also, do you know how to check that ACCESS CO., LTD. signed a corporate NDA?
LGTM Confirmed presence of CLA signature.
The CQ bit was checked by dgrogan@chromium.org
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": 40001, "attempt_start_ts": 1493242463995890,
"parent_rev": "dd7eb407434e63553da0cd20ec865272a562ba3c", "commit_rev":
"5cdd84c272ce41e804a9d4447b288f44d748c6af"}
Message was sent while issue was closed.
Description was changed from ========== LayoutTableCell::OffsetFromContainer() should use the flipped offset of its parent. Since LayoutTableCell is relative to LayoutTableSection, it should subtract the offset of the LayoutTableRow. OffsetFromContainer() is flipped value, but LayoutTableCell::OffsetFromContainer() uses LocationOffset() which is an unflipped location. Therefore, the rect of td goes outside of tr. It should use PhysicalLocationOffset() which returns a flipped location. BUG=711193 ========== to ========== LayoutTableCell::OffsetFromContainer() should use the flipped offset of its parent. Since LayoutTableCell is relative to LayoutTableSection, it should subtract the offset of the LayoutTableRow. OffsetFromContainer() is flipped value, but LayoutTableCell::OffsetFromContainer() uses LocationOffset() which is an unflipped location. Therefore, the rect of td goes outside of tr. It should use PhysicalLocationOffset() which returns a flipped location. BUG=711193 Review-Url: https://codereview.chromium.org/2816983002 Cr-Commit-Position: refs/heads/master@{#467467} Committed: https://chromium.googlesource.com/chromium/src/+/5cdd84c272ce41e804a9d4447b28... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5cdd84c272ce41e804a9d4447b28... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
