Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1819073004:
Fix table cell background painting (Closed)
|
Created:
4 years, 9 months ago by atotic1 Modified:
4 years, 8 months ago Reviewers:
Xianzhu CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, slimming-paint-reviews_chromium.org, dshwang, jchaffraix+rendering, blink-reviews-paint_chromium.org, blink-reviews, kinuko+watch, dgrogan Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix table cell background painting
Table cell backgrounds inherited from tbody, colgroup, col, and tr
were painted incorrectly, just blasted into cellRect.
Biggest part of the fix was determining position of tbody, cols, tr.
LayoutTableSection::getCellPhysicalPosition provides the way to
determine position for all of them.
<col> required a lot of tricky conversions between LayoutTableCol
and effectiveColIndexes.
As recommended by wangxianzhu, the fix is behind
NewTableCellBackgroundPainting flag, and can be switched off.
BUG=125336
Patch Set 1 #
Total comments: 60
Patch Set 2 : cr fixes, remove tiling algorithm #Messages
Total messages: 18 (4 generated)
Description was changed from ========== Fix table cell background painting Table cell backgrounds inherited from tbody, colgroup, col, and tr were painted incorrectly, just blasted into cellRect. Biggest part of the fix was determining position of tbody, cols, tr. LayoutTableSection::getCellPhysicalPosition provides the way to determine position for all of them. <col> required a lot of tricky conversions between LayoutTableCol and effectiveColIndexes. As recommended by xianzhu, the fix is behind NewTableCellBackgroundPainting flag, and can be switched off. BUG=125336 ========== to ========== Fix table cell background painting Table cell backgrounds inherited from tbody, colgroup, col, and tr were painted incorrectly, just blasted into cellRect. Biggest part of the fix was determining position of tbody, cols, tr. LayoutTableSection::getCellPhysicalPosition provides the way to determine position for all of them. <col> required a lot of tricky conversions between LayoutTableCol and effectiveColIndexes. As recommended by wangxianzhu, the fix is behind NewTableCellBackgroundPainting flag, and can be switched off. BUG=125336 ==========
atotic@google.com changed reviewers: + wangxianzhu@chromium.org
The CQ bit was checked by atotic@google.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/1819073004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819073004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/03/22 20:39:01, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1819073004/1 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1819073004/1 Two tests were unexpected failures, but I do not think my patch was the cause: inspector/elements/shadow/elements-panel-shadow-selection-on-refresh.html inspector/elements/edit/edit-dom-actions.html It looks like it relates to ShadowDOMv1 migration. The rest of the failures were expected. If you compare results to FF, they are almost identical (except our rowspan border bugs).
Please review my cl. It fixes the bug, and hides the fix behind the flag.
Please skip to https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... first. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.cpp:856: // an inverse of slowColElementAtAbsoluteColumn. Move the comment to header file. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.cpp:857: unsigned LayoutTable::colElementToAbsoluteColumn(const LayoutTableCol * colElement) const Nit: remove the extra space before '*'. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.cpp:863: break; Change this to 'return col'; https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.cpp:865: // Do nothing, span will increase with children The comment is not clear to me. How about: // Count spans of leaf col elements only. if (!columnLayoutObject->isTableColumnGroupWithColumnChildren()) col += columnLayoutObject->span(); https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.cpp:870: return col; Change this to: ASSERT_NOT_REACHED(); return 0; to catch errors. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTable.h (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.h:340: unsigned colElementToAbsoluteColumn(const LayoutTableCol *) const; Nit: Remove the extra space before '*'. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:191: next = p->nextSibling(); Is this change necessary? https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableCol.h (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableCol.h:97: // <col> elements need to provide absoluteColumnIndex. Can you rephrase the above sentence? https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableCol.h:101: // particular absolute column. I think you can omit the above comments. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableRow.h (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableRow.h:136: LayoutRect positionByCellSpan() const; Please add documentation for this method. I can't guess what this function does from its name. Perhaps you should also give it a better name. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1668: LayoutRect LayoutTableSection::transformLogicalToPhysicalPosition(const LayoutRect& position) const Can you use LayoutBox::flipForWritingMode() in this function? https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.h:317: LayoutRect getCellPosition(unsigned row, unsigned effectiveColumn) const; Document which coordinate space the returned value is in. Also don't use 'transformation' whose defined meaning is different from what you means. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.h:320: LayoutRect getCellPhysicalPosition(unsigned row, unsigned effectiveColumn) const; Don't use 'transform'. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.h:323: LayoutRect positionByCellSpan() const; What does "ByCellSpan" mean? https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.h:376: LayoutRect transformLogicalToPhysicalPosition(const LayoutRect& position) const; Don't use 'transform'. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:197: LayoutTable* tableElt = m_layoutTableCell.table(); s/tableElt/table/ https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:212: // painting directions correctly. Convert the above to a TODO, and reference a bug. You can use TODO(crbug.com/...): style if you are not going to fix it. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:217: backgroundRect.setY(cellRect.y()); Why does this happen? https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:219: Optional<LayoutObjectDrawingRecorder> recorder; This recorder doesn't need to be optional. You can declare and initialize it at the same time at line 223. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:225: Color c = backgroundObject->resolveColor(CSSPropertyBackgroundColor); s/c/color/ https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:226: const FillLayer& bgLayer = backgroundObject->style()->backgroundLayers(); s/bgLayer/backgroundLayer/ https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:227: FillLayer bgAdjustedLayer(bgLayer); Not used? https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:247: } The following case is broken with the code: <!DOCTYPE html> <style> td { width: 100px; height: 100px; border: 2px solid red; } </style> <table> <tr style="background: url(https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png)"> <td rowspan="2">11</td> <td>12</td> </tr> <tr> <td>22</td> </tr> </table> The '11' cell should have continuous background spanning 2 rows. It displays wrong with the new code, but correct with the original code. (Your code does fix the background of the second cell.) I think you don't need the above loop, and nor the size part of the rect returned from 'positionByCellSpan()' methods. You just need to adjust the offset of the background. In this way, you don't need positionByCellSpan() methods, but just the offset of the cell from the backgroundObject, and the CL can be greatly simplified. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:315: { Put '{' at the end of the previous line. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:322: } Put break inside of '}' and align '}' with 'case'. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:324: case DisplayItem::TableCellBackgroundFromSection: We know TableCellSection's paint bounds in TableCellPainter. You can just pass it from the callsite into TableCellPaint::paintBackgroundsBehindCell(). In this way, you don't need LayoutTableSection::positionByCellSpan(). https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:331: break; Ditto. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:339: ASSERT(false); ASSERT_NOT_REACHED(); https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/TableCellPainter.h (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.h:40: void paintParentBackgroundsBehindCell(const PaintInfo&, const LayoutPoint&, const LayoutBox* backgroundObject, DisplayItem::Type); 1. Make it private. 2. backgroundObject should be 'const LayoutBox&' because it is never null. You can update paintBackgroundsBehindCell also accept 'const LayoutBox&' or leave it until you remove the RuntimeEnabledFeature. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.h:43: LayoutRect paintBoundsParent(const LayoutPoint& paintOffset, const LayoutBox*, DisplayItem::Type); Make it private.
Good catch. I was focusing on explicitly sized backgrounds, and did not test the unsized case. There is no easy fix, please read comment for details. I'll address the rest of code review shortly. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:247: } On 2016/03/28 00:44:43, Xianzhu wrote: > The following case is broken with the code: > > <!DOCTYPE html> > <style> > td { > width: 100px; > height: 100px; > border: 2px solid red; > } > </style> > <table> > <tr style="background: > url(https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png)"> > <td rowspan="2">11</td> > <td>12</td> > </tr> > <tr> > <td>22</td> > </tr> > </table> > > > The '11' cell should have continuous background spanning 2 rows. It displays > wrong with the new code, but correct with the original code. (Your code does fix > the background of the second cell.) You are correct in that we fail this test case. My messy code was trying to approximate background image tiling, which is what BoxPainter also does. Initially, I tried using BoxPainter for tiling, and it did not work. The proper fix is to fix BoxPainter API so it can handle this use case. BoxPainter is undocumented. This is my understanding of the API, and I'd love to learn that I am wrong, and there is a call I can use without modification. I'll use <tr> to illustrate the problem: - tr image background has to be painted inside the entire cell. It needs to support all the usual background-image properties. Example 1: background-image-size: 100% 100%; should paint an image that is 100% of the size of the row - if the cell size extends beyond row size, background should fill the extended area. This cannot be done with BoxPainter API today because: BoxPainter::paintFillLayers(const PaintInfo& paintInfo, const Color& c, const FillLayer& fillLayer, const LayoutRect& paintRect, BackgroundBleedAvoidance bleedAvoidance, SkXfermode::Mode op, const LayoutObject* backgroundObject) uses paintRect for: 1) Image scaling: paintRect is a containment rectangle that determines image size 2) Paint destination: paintRect is the area to be painted Simply put, there is no way to use <tr> frame for image scaling, and cell frame for image painting in current API. For this to be fixed, BoxPainter::paintFillLayer needs to be modified to allow separate rectangles: one for image scaling, another as paint destination. This will be a slightly larger change, there are 7 calls to paintFillLayer in our code. I see no other way to paint tiled backgrounds correctly. The code that handles all permutations of background-image properties is very complex, and I do not want to reinvent it. > I think you don't need the above loop, and nor the size part of the rect > returned from 'positionByCellSpan()' methods. You just need to adjust the > offset of the background. In this way, you don't need positionByCellSpan() > methods, but just the offset of the cell from the backgroundObject, and the CL > can be greatly simplified. To paint the background of every cell correctly, I need two things: - background size - background origin with respect to cell How can I paint the background correctly without knowing the its size? The tiling properties need to know element's size.
https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:247: } On 2016/03/28 20:45:33, atotic1 wrote: > On 2016/03/28 00:44:43, Xianzhu wrote: > > The following case is broken with the code: > > > > <!DOCTYPE html> > > <style> > > td { > > width: 100px; > > height: 100px; > > border: 2px solid red; > > } > > </style> > > <table> > > <tr style="background: > > > url(https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png)"> > > <td rowspan="2">11</td> > > <td>12</td> > > </tr> > > <tr> > > <td>22</td> > > </tr> > > </table> > > > > > > The '11' cell should have continuous background spanning 2 rows. It displays > > wrong with the new code, but correct with the original code. (Your code does > fix > > the background of the second cell.) > > You are correct in that we fail this test case. My messy code was trying to > approximate > background image tiling, which is what BoxPainter also does. Initially, I tried > using BoxPainter for tiling, and it did not work. > > The proper fix is to fix BoxPainter API so it can handle this use case. > > BoxPainter is undocumented. This is my understanding of the API, > and I'd love to learn that I am wrong, and there is a call I > can use without modification. > > I'll use <tr> to illustrate the problem: > - tr image background has to be painted inside the entire cell. > It needs to support all the usual background-image properties. > Example 1: > background-image-size: 100% 100%; > should paint an image that is 100% of the size of the row > - if the cell size extends beyond row size, background should > fill the extended area. > > This cannot be done with BoxPainter API today because: > > BoxPainter::paintFillLayers(const PaintInfo& paintInfo, > const Color& c, > const FillLayer& fillLayer, > const LayoutRect& paintRect, > BackgroundBleedAvoidance bleedAvoidance, > SkXfermode::Mode op, > const LayoutObject* backgroundObject) > > uses paintRect for: > 1) Image scaling: paintRect is a containment rectangle that determines image > size > 2) Paint destination: paintRect is the area to be painted > > Simply put, there is no way to use <tr> frame for image scaling, and cell frame > for image painting in current API. > > For this to be fixed, BoxPainter::paintFillLayer needs to be modified to allow > separate rectangles: one for image scaling, another as paint destination. > > This will be a slightly larger change, there are 7 calls to paintFillLayer > in our code. > > I see no other way to paint tiled backgrounds correctly. The code that handles > all permutations of background-image properties is very complex, and I do > not want to reinvent it. > Because the background bug has been there for a long time, many developers have learned to avoid it. If a fix to a bug is to introduce new bugs, we'd rather not to fix the bug. Unsized background may be more common than sized background, so we should not break it. Please modify paintFillLayer to accept separate rectangles if you would like to continue the work to fix the bug. > > I think you don't need the above loop, and nor the size part of the rect > > returned from 'positionByCellSpan()' methods. You just need to adjust the > > offset of the background. In this way, you don't need positionByCellSpan() > > methods, but just the offset of the cell from the backgroundObject, and the CL > > can be greatly simplified. > > To paint the background of every cell correctly, I need two things: > - background size > - background origin with respect to cell > > How can I paint the background correctly without knowing the its size? > The tiling properties need to know element's size. You are right. Sorry I overlooked the percentage-sized background case.
On 2016/03/28 21:26:04, Xianzhu wrote: > https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right): > > https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/paint/TableCellPainter.cpp:247: } > On 2016/03/28 20:45:33, atotic1 wrote: > > On 2016/03/28 00:44:43, Xianzhu wrote: > > > The following case is broken with the code: > > > > > > <!DOCTYPE html> > > > <style> > > > td { > > > width: 100px; > > > height: 100px; > > > border: 2px solid red; > > > } > > > </style> > > > <table> > > > <tr style="background: > > > > > > url(https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png)"> > > > <td rowspan="2">11</td> > > > <td>12</td> > > > </tr> > > > <tr> > > > <td>22</td> > > > </tr> > > > </table> > > > > > > > > > The '11' cell should have continuous background spanning 2 rows. It displays > > > wrong with the new code, but correct with the original code. (Your code does > > fix > > > the background of the second cell.) > > > > You are correct in that we fail this test case. My messy code was trying to > > approximate > > background image tiling, which is what BoxPainter also does. Initially, I > tried > > using BoxPainter for tiling, and it did not work. > > > > The proper fix is to fix BoxPainter API so it can handle this use case. > > > > BoxPainter is undocumented. This is my understanding of the API, > > and I'd love to learn that I am wrong, and there is a call I > > can use without modification. > > > > I'll use <tr> to illustrate the problem: > > - tr image background has to be painted inside the entire cell. > > It needs to support all the usual background-image properties. > > Example 1: > > background-image-size: 100% 100%; > > should paint an image that is 100% of the size of the row > > - if the cell size extends beyond row size, background should > > fill the extended area. > > > > This cannot be done with BoxPainter API today because: > > > > BoxPainter::paintFillLayers(const PaintInfo& paintInfo, > > const Color& c, > > const FillLayer& fillLayer, > > const LayoutRect& paintRect, > > BackgroundBleedAvoidance bleedAvoidance, > > SkXfermode::Mode op, > > const LayoutObject* backgroundObject) > > > > uses paintRect for: > > 1) Image scaling: paintRect is a containment rectangle that determines image > > size > > 2) Paint destination: paintRect is the area to be painted > > > > Simply put, there is no way to use <tr> frame for image scaling, and cell > frame > > for image painting in current API. > > > > For this to be fixed, BoxPainter::paintFillLayer needs to be modified to allow > > separate rectangles: one for image scaling, another as paint destination. > > > > This will be a slightly larger change, there are 7 calls to paintFillLayer > > in our code. > > > > I see no other way to paint tiled backgrounds correctly. The code that handles > > all permutations of background-image properties is very complex, and I do > > not want to reinvent it. > > > > Because the background bug has been there for a long time, many developers have > learned to avoid it. If a fix to a bug is to introduce new bugs, we'd rather not > to fix the bug. Unsized background may be more common than sized background, so > we should not break it. > > Please modify paintFillLayer to accept separate rectangles if you would like to > continue the work to fix the bug. > > > > I think you don't need the above loop, and nor the size part of the rect > > > returned from 'positionByCellSpan()' methods. You just need to adjust the > > > offset of the background. In this way, you don't need positionByCellSpan() > > > methods, but just the offset of the cell from the backgroundObject, and the > CL > > > can be greatly simplified. > > > > To paint the background of every cell correctly, I need two things: > > - background size > > - background origin with respect to cell > > > > How can I paint the background correctly without knowing the its size? > > The tiling properties need to know element's size. > > You are right. Sorry I overlooked the percentage-sized background case. I've looked into extending BoxPainter, it is beyond what I can do in a month, 300 lines of very particular code. I think this job is better suited for someone who is familiar with BoxPainter (paint team?). Instead, I propose a simpler solution. By unifying cellRect and backgroundRect - everything works in all instances when rowspan/colspan are 1 - otherwise has a bug that stretches images accross the entire cell if image size is in percent
On 2016/03/29 00:42:55, atotic1 wrote: > On 2016/03/28 21:26:04, Xianzhu wrote: > > > https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right): > > > > > https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/paint/TableCellPainter.cpp:247: } > > On 2016/03/28 20:45:33, atotic1 wrote: > > > On 2016/03/28 00:44:43, Xianzhu wrote: > > > > The following case is broken with the code: > > > > > > > > <!DOCTYPE html> > > > > <style> > > > > td { > > > > width: 100px; > > > > height: 100px; > > > > border: 2px solid red; > > > > } > > > > </style> > > > > <table> > > > > <tr style="background: > > > > > > > > > > url(https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png)"> > > > > <td rowspan="2">11</td> > > > > <td>12</td> > > > > </tr> > > > > <tr> > > > > <td>22</td> > > > > </tr> > > > > </table> > > > > > > > > > > > > The '11' cell should have continuous background spanning 2 rows. It > displays > > > > wrong with the new code, but correct with the original code. (Your code > does > > > fix > > > > the background of the second cell.) > > > > > > You are correct in that we fail this test case. My messy code was trying to > > > approximate > > > background image tiling, which is what BoxPainter also does. Initially, I > > tried > > > using BoxPainter for tiling, and it did not work. > > > > > > The proper fix is to fix BoxPainter API so it can handle this use case. > > > > > > BoxPainter is undocumented. This is my understanding of the API, > > > and I'd love to learn that I am wrong, and there is a call I > > > can use without modification. > > > > > > I'll use <tr> to illustrate the problem: > > > - tr image background has to be painted inside the entire cell. > > > It needs to support all the usual background-image properties. > > > Example 1: > > > background-image-size: 100% 100%; > > > should paint an image that is 100% of the size of the row > > > - if the cell size extends beyond row size, background should > > > fill the extended area. > > > > > > This cannot be done with BoxPainter API today because: > > > > > > BoxPainter::paintFillLayers(const PaintInfo& paintInfo, > > > const Color& c, > > > const FillLayer& fillLayer, > > > const LayoutRect& paintRect, > > > BackgroundBleedAvoidance bleedAvoidance, > > > SkXfermode::Mode op, > > > const LayoutObject* backgroundObject) > > > > > > uses paintRect for: > > > 1) Image scaling: paintRect is a containment rectangle that determines image > > > size > > > 2) Paint destination: paintRect is the area to be painted > > > > > > Simply put, there is no way to use <tr> frame for image scaling, and cell > > frame > > > for image painting in current API. > > > > > > For this to be fixed, BoxPainter::paintFillLayer needs to be modified to > allow > > > separate rectangles: one for image scaling, another as paint destination. > > > > > > This will be a slightly larger change, there are 7 calls to paintFillLayer > > > in our code. > > > > > > I see no other way to paint tiled backgrounds correctly. The code that > handles > > > all permutations of background-image properties is very complex, and I do > > > not want to reinvent it. > > > > > > > Because the background bug has been there for a long time, many developers > have > > learned to avoid it. If a fix to a bug is to introduce new bugs, we'd rather > not > > to fix the bug. Unsized background may be more common than sized background, > so > > we should not break it. > > > > Please modify paintFillLayer to accept separate rectangles if you would like > to > > continue the work to fix the bug. > > > > > > I think you don't need the above loop, and nor the size part of the rect > > > > returned from 'positionByCellSpan()' methods. You just need to adjust the > > > > offset of the background. In this way, you don't need positionByCellSpan() > > > > methods, but just the offset of the cell from the backgroundObject, and > the > > CL > > > > can be greatly simplified. > > > > > > To paint the background of every cell correctly, I need two things: > > > - background size > > > - background origin with respect to cell > > > > > > How can I paint the background correctly without knowing the its size? > > > The tiling properties need to know element's size. > > > > You are right. Sorry I overlooked the percentage-sized background case. > > I've looked into extending BoxPainter, it is beyond what I can do in a month, > 300 lines of very particular code. I think this job is better suited for > someone who is familiar with BoxPainter (paint team?). > > Instead, I propose a simpler solution. By unifying cellRect and backgroundRect > - everything works in all instances when rowspan/colspan are 1 > - otherwise has a bug that stretches images accross the entire cell if image > size is in percent This plan sounds good to me. I suggest you to avoid the positionForCellSpan() methods, but pass the offset between the background origin and the cell to paintBackgroundBindCell instead.
On 2016/03/29 02:30:00, Xianzhu wrote: > This plan sounds good to me. I suggest you to avoid the positionForCellSpan() > methods, but pass the offset between the background origin and the cell to > paintBackgroundBindCell instead. positionForCellSpan() has been renamed to positionForBackgroundPaint() I need to know size of the background for painting. How do I get that out of the offset?
On 2016/03/29 03:38:09, atotic2 wrote: > On 2016/03/29 02:30:00, Xianzhu wrote: > > This plan sounds good to me. I suggest you to avoid the positionForCellSpan() > > methods, but pass the offset between the background origin and the cell to > > paintBackgroundBindCell instead. > > positionForCellSpan() has been renamed to positionForBackgroundPaint() > > I need to know size of the background for painting. How do I get > that out of the offset? I noticed that what I suggested was to step backwards more than your plan: just to fix the offset issue of non-precent-sized background. Now I know your plan is to also fix percent-sized background, and to stretch the background if the cell spans bigger than the background object. However, this will make what was wrong still wrong in a new way, which is not desired. What I could suggest is limited. You might want to get more opinions about this. As your change will affect blink's "open web" behavior, an email to blink-dev@chromium.org with subject like "Intent to Implement: new table cell background painting" might be appropriate way to get more opinions from API owners and other developers. See http://www.chromium.org/blink#launch-process for the process of an Intent.
On 2016/03/29 04:26:17, Xianzhu wrote: > I noticed that what I suggested was to step backwards more than your plan: just > to fix the offset issue of non-precent-sized background. Now I know your plan is > to also fix percent-sized background, and to stretch the background if the cell > spans bigger than the background object. However, this will make what was wrong > still wrong in a new way, which is not desired. > > What I could suggest is limited. You might want to get more opinions about this. > As your change will affect blink's "open web" behavior, an email to > mailto:blink-dev@chromium.org with subject like "Intent to Implement: new table cell > background painting" might be appropriate way to get more opinions from API > owners and other developers. See http://www.chromium.org/blink#launch-process > for the process of an Intent. The original bug was to fix painting of gradient background. Gradient backgrounds sizes are always percent-sized. Taking out percentage-sized leaves the original bug still open. I will upload my latest set of code-review changes, and start the Intent To Implement process.
On 2016/03/29 16:52:46, atotic1 wrote: > On 2016/03/29 04:26:17, Xianzhu wrote: > > I noticed that what I suggested was to step backwards more than your plan: > just > > to fix the offset issue of non-precent-sized background. Now I know your plan > is > > to also fix percent-sized background, and to stretch the background if the > cell > > spans bigger than the background object. However, this will make what was > wrong > > still wrong in a new way, which is not desired. > > > > What I could suggest is limited. You might want to get more opinions about > this. > > As your change will affect blink's "open web" behavior, an email to > > mailto:blink-dev@chromium.org with subject like "Intent to Implement: new > table cell > > background painting" might be appropriate way to get more opinions from API > > owners and other developers. See http://www.chromium.org/blink#launch-process > > for the process of an Intent. > > The original bug was to fix painting of gradient background. Gradient > backgrounds > sizes are always percent-sized. Taking out percentage-sized leaves the original > bug still open. > > I will upload my latest set of code-review changes, and start the Intent To > Implement > process. I will not review the CL unless: 1. it is fully correct (e.g. with BoxPainter::paintFillLayers supporting backgroundRect and paintRect); or 2. it doesn't fix all bugs, but it won't cause any regressions including changing wrong behavior into another wrong way. If you would like to create a CL between 1 and 2, please either - go through Intent to Implement process and get approval to change behavior or - feel free to get other reviewers to review your CL.
Checked in the code review changes. Next step is to start "Intent To Implement" process. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.cpp:856: // an inverse of slowColElementAtAbsoluteColumn. On 2016/03/28 00:44:42, Xianzhu wrote: > Move the comment to header file. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.cpp:857: unsigned LayoutTable::colElementToAbsoluteColumn(const LayoutTableCol * colElement) const On 2016/03/28 00:44:42, Xianzhu wrote: > Nit: remove the extra space before '*'. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.cpp:863: break; On 2016/03/28 00:44:42, Xianzhu wrote: > Change this to 'return col'; Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.cpp:865: // Do nothing, span will increase with children On 2016/03/28 00:44:42, Xianzhu wrote: > The comment is not clear to me. How about: > // Count spans of leaf col elements only. > if (!columnLayoutObject->isTableColumnGroupWithColumnChildren()) > col += columnLayoutObject->span(); Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.cpp:870: return col; On 2016/03/28 00:44:41, Xianzhu wrote: > Change this to: > ASSERT_NOT_REACHED(); > return 0; > to catch errors. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTable.h (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTable.h:340: unsigned colElementToAbsoluteColumn(const LayoutTableCol *) const; On 2016/03/28 00:44:42, Xianzhu wrote: > Nit: Remove the extra space before '*'. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:191: next = p->nextSibling(); On 2016/03/28 00:44:42, Xianzhu wrote: > Is this change necessary? I've seen parent() be null while debugging. That's why I put the guard in. Happy to take it out if you'd like. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableCol.h (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableCol.h:97: // <col> elements need to provide absoluteColumnIndex. On 2016/03/28 00:44:42, Xianzhu wrote: > Can you rephrase the above sentence? Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableCol.h:101: // particular absolute column. On 2016/03/28 00:44:42, Xianzhu wrote: > I think you can omit the above comments. Moved it into .cpp. The comment describes a non-intuitive aspect of table spec. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableRow.h (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableRow.h:136: LayoutRect positionByCellSpan() const; On 2016/03/28 00:44:42, Xianzhu wrote: > Please add documentation for this method. I can't guess what this function does > from its name. Perhaps you should also give it a better name. I've renamed positionByCellSpan to "positionForBackgroundDrawing" https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1668: LayoutRect LayoutTableSection::transformLogicalToPhysicalPosition(const LayoutRect& position) const On 2016/03/28 00:44:42, Xianzhu wrote: > Can you use LayoutBox::flipForWritingMode() in this function? No. The transformation computed here is more complex. This is the simplest I could make it. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.h:317: LayoutRect getCellPosition(unsigned row, unsigned effectiveColumn) const; On 2016/03/28 00:44:42, Xianzhu wrote: > Document which coordinate space the returned value is in. > > Also don't use 'transformation' whose defined meaning is different from what you > means. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.h:320: LayoutRect getCellPhysicalPosition(unsigned row, unsigned effectiveColumn) const; On 2016/03/28 00:44:42, Xianzhu wrote: > Don't use 'transform'. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.h:320: LayoutRect getCellPhysicalPosition(unsigned row, unsigned effectiveColumn) const; On 2016/03/28 00:44:42, Xianzhu wrote: > Don't use 'transform'. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.h:376: LayoutRect transformLogicalToPhysicalPosition(const LayoutRect& position) const; On 2016/03/28 00:44:42, Xianzhu wrote: > Don't use 'transform'. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:197: LayoutTable* tableElt = m_layoutTableCell.table(); On 2016/03/28 00:44:43, Xianzhu wrote: > s/tableElt/table/ Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:212: // painting directions correctly. On 2016/03/28 00:44:42, Xianzhu wrote: > Convert the above to a TODO, and reference a bug. You can use > TODO(crbug.com/...): style if you are not going to fix it. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:217: backgroundRect.setY(cellRect.y()); On 2016/03/28 00:44:43, Xianzhu wrote: > Why does this happen? This happens when table is vertical. I've reworked the code to be much simpler. It will be incorrect, see referenced bug for more details. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:219: Optional<LayoutObjectDrawingRecorder> recorder; On 2016/03/28 00:44:42, Xianzhu wrote: > This recorder doesn't need to be optional. You can declare and initialize it at > the same time at line 223. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:225: Color c = backgroundObject->resolveColor(CSSPropertyBackgroundColor); On 2016/03/28 00:44:42, Xianzhu wrote: > s/c/color/ Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:226: const FillLayer& bgLayer = backgroundObject->style()->backgroundLayers(); On 2016/03/28 00:44:43, Xianzhu wrote: > s/bgLayer/backgroundLayer/ Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:227: FillLayer bgAdjustedLayer(bgLayer); On 2016/03/28 00:44:42, Xianzhu wrote: > Not used? Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:315: { On 2016/03/28 00:44:43, Xianzhu wrote: > Put '{' at the end of the previous line. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:322: } On 2016/03/28 00:44:43, Xianzhu wrote: > Put break inside of '}' and align '}' with 'case'. Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:324: case DisplayItem::TableCellBackgroundFromSection: On 2016/03/28 00:44:42, Xianzhu wrote: > We know TableCellSection's paint bounds in TableCellPainter. You can just pass > it from the callsite into TableCellPaint::paintBackgroundsBehindCell(). In this > way, you don't need LayoutTableSection::positionByCellSpan(). How do we know TableSection bounds inside TableCellPainter? https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:339: ASSERT(false); On 2016/03/28 00:44:42, Xianzhu wrote: > ASSERT_NOT_REACHED(); Done. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/TableCellPainter.h (right): https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.h:40: void paintParentBackgroundsBehindCell(const PaintInfo&, const LayoutPoint&, const LayoutBox* backgroundObject, DisplayItem::Type); On 2016/03/28 00:44:43, Xianzhu wrote: > 1. Make it private. > 2. backgroundObject should be 'const LayoutBox&' because it is never null. You > can update paintBackgroundsBehindCell also accept 'const LayoutBox&' or leave it > until you remove the RuntimeEnabledFeature. Done. Modified the old routine to accept const LayoutBox& too. https://codereview.chromium.org/1819073004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/TableCellPainter.h:43: LayoutRect paintBoundsParent(const LayoutPoint& paintOffset, const LayoutBox*, DisplayItem::Type); On 2016/03/28 00:44:43, Xianzhu wrote: > Make it private. Done. |
