Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1368243002:
Don't deduct min-width from the available width for empty cells (Closed)
|
Created:
5 years, 2 months ago by rhogan Modified:
5 years, 2 months ago Reviewers:
mstensho (USE GERRIT) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't deduct min-width from the available width for empty cells
If a column has cells, even if they are empty cells, then their full width needs
to be included in the width available for distribution to columns in the table.
This was an oversight when I introduced zero-width empty cells.
BUG=534830
Committed: https://crrev.com/5693a8522953e3511d31fd915716628b202d56f7
Cr-Commit-Position: refs/heads/master@{#352253}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Updated #Patch Set 3 : Updated #Patch Set 4 : Updated #
Messages
Total messages: 26 (11 generated)
robhogan@gmail.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/1368243002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/table/colspan-with-empty-cells-needing-extra-width.html (right): https://codereview.chromium.org/1368243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/table/colspan-with-empty-cells-needing-extra-width.html:5: margin: 0; No UA-defined margins on those elements. https://codereview.chromium.org/1368243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/table/colspan-with-empty-cells-needing-extra-width.html:8: <p>crbug.com/534830: Don't deduct the min-width of empty cells from the available table width. </p> Can you please write a test with a human readable and observable pass condition? It also seems that it should be easy to write a check-layout.js test instead. Those run much faster. https://codereview.chromium.org/1368243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/table/colspan-with-empty-cells-needing-extra-width.html:9: <table style="width: 800px; background-color: black;"> This is wider than the initial viewport when starting content_shell. Use width:100% instead? https://codereview.chromium.org/1368243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/table/colspan-with-empty-cells-needing-extra-width.html:11: <td style="background-color:red; width: 10px; height:20px;"></td> Red means danger. Maybe set the background of the table to red instead, set its border-spacing to 0 and have "no red" as part of the pass condition? https://codereview.chromium.org/1368243002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp (right): https://codereview.chromium.org/1368243002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:603: available += allocAuto; |available| already contains |allocAuto|, if |numAuto| (a few lines up). Maybe we should never enter both blocks, so that the code should better read: if (available > 0 && (numAuto || numAutoEmptyCellsOnly)) { available += allocAuto; if (numAuto) { distributeWidthToColumns<float, Auto, NonEmptyCells, InitialWidth, StartToEnd>(available, totalAuto); } else { unsigned total = numAutoEmptyCellsOnly; distributeWidthToColumns<unsigned, Auto, EmptyCells, InitialWidth, StartToEnd>(available, total); } }
On 2015/09/30 at 11:09:39, mstensho wrote: > https://codereview.chromium.org/1368243002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp:603: available += allocAuto; > |available| already contains |allocAuto|, if |numAuto| (a few lines up). > Updated everything - incorporated your change without the 'else' as need to try both branches.
lgtm
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368243002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368243002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by robhogan@gmail.com
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/1368243002/#ps60001 (title: "Updated")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368243002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368243002/60001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368243002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368243002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368243002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368243002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368243002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368243002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5693a8522953e3511d31fd915716628b202d56f7 Cr-Commit-Position: refs/heads/master@{#352253} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
