Full Text Bug Listing

Allan Sandfeld Jensen

Reported 2012-05-16 03:32:55 PDT

When doing rect-based hit-testing on a table without overflow, only the cell directly under the center point will be hit-tested and not all the cells that intersects with the area under the hit-test point.

Allan Sandfeld Jensen

Comment 1 2012-05-16 03:39:10 PDT

Created attachment 142212 [details] Patch

Allan Sandfeld Jensen

Comment 2 2012-05-25 05:09:56 PDT

Created attachment 144048 [details] Patch Rebase to not depend on patch for bug 85792

Julien Chaffraix

Comment 3 2012-05-25 13:13:41 PDT

Comment on attachment 144048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144048&action=review > Source/WebCore/ChangeLog:9 > + After binary look-up of row and cell, continue to test following > + cells that intersect with the area of the hit-test point. This is not a useful description of what you are fixing nor really how. Btw, you do a binary search on the rows and *columns*, not cells. > Source/WebCore/rendering/RenderTableSection.cpp:1346 > + LayoutUnit offsetStartInColumnDirection, offsetEndInColumnDirection; This is not the rule in WebKit to define 2 variables on one line. Those should be split on 2 lines. > Source/WebCore/rendering/RenderTableSection.cpp:1363 > + if (style()->isHorizontalWritingMode()) { > + if (!style()->isFlippedBlocksWritingMode()) { > + offsetStartInColumnDirection = hitTestRect.y(); > + offsetEndInColumnDirection = hitTestRect.maxY(); > + } else { > + offsetStartInColumnDirection = height() - hitTestRect.maxY(); > + offsetEndInColumnDirection = height() - hitTestRect.y(); > + } > + } else { > + if (!style()->isFlippedBlocksWritingMode()) { > + offsetStartInColumnDirection = hitTestRect.x(); > + offsetEndInColumnDirection = hitTestRect.maxX(); > + } else { > + offsetStartInColumnDirection = width() - hitTestRect.maxX(); > + offsetEndInColumnDirection = width() - hitTestRect.x(); > + } > } AFAICT this should use the table's style as we don't allow sections to have a writing mode different from the table. Also why is this not using RenderBox::flipForWritingMode? > Source/WebCore/rendering/RenderTableSection.cpp:1392 > + } Same 2 comments as above. > Source/WebCore/rendering/RenderTableSection.cpp:1417 > + ++hitColumn; The inner loop should go into its own function. That should make the code more readable. > Source/WebCore/rendering/RenderTableSection.cpp:1418 > + } while (hitColumn < (columnPos.size() - 1) && columnPos[hitColumn] <= offsetEndInRowDirection); How is a do ... while any better than a for? For the record, this made the code so confusing that I missed that you kept the same direction of look-ups above. > LayoutTests/fast/dom/nodesFromRect-table.html:1 > +<html> Please add the proper doctype as you are not testing quirks mode: <!DOCTYPE html> > LayoutTests/fast/dom/nodesFromRect-table.html:22 > +<body id="body"> is this id used anywhere? > LayoutTests/fast/dom/nodesFromRect-table.html:48 > + <script type="application/javascript"> No need for the type, this is HTML. > LayoutTests/fast/dom/nodesFromRect-table.html:53 > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); You don't need those 2 lines: js-test-pre.js already calls the former, the latter is unneeded as we stop the test _after_ the load event handler. > LayoutTests/fast/dom/nodesFromRect-table.html:85 > + check(190, 175, 10, 20, 10, 20, [e.td22, e.td23, e.testtable]); > + check(260, 275, 10, 20, 10, 20, [e.td43, e.td44, e.testtable]); > + check(175, 190, 20, 10, 20, 10, [e.td22, e.td32, e.testtable]); > + check(275, 260, 20, 10, 20, 10, [e.td34, e.td44, e.testtable]); > + check(190, 190, 20, 20, 20, 20, [e.td22, e.td23, e.td32, e.td33, e.testtable]); Interestingly you don't get the rows, I am not 100% this is expected here as you have some spacing between your cells. I think you should define a table section by adding a <tbody id="tbody"> around your rows (here it's anonymous and maybe that's why you don't see it in your results). > LayoutTests/fast/dom/nodesFromRect-table.html:98 > + if (window.layoutTestController) > + layoutTestController.notifyDone(); This is unneeded too.

Allan Sandfeld Jensen

Comment 4 2012-05-29 02:41:10 PDT

(In reply to comment #3) > (From update of attachment 144048 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144048&action=review > > > Source/WebCore/rendering/RenderTableSection.cpp:1363 > > + if (style()->isHorizontalWritingMode()) { > > + if (!style()->isFlippedBlocksWritingMode()) { > > + offsetStartInColumnDirection = hitTestRect.y(); > > + offsetEndInColumnDirection = hitTestRect.maxY(); > > + } else { > > + offsetStartInColumnDirection = height() - hitTestRect.maxY(); > > + offsetEndInColumnDirection = height() - hitTestRect.y(); > > + } > > + } else { > > + if (!style()->isFlippedBlocksWritingMode()) { > > + offsetStartInColumnDirection = hitTestRect.x(); > > + offsetEndInColumnDirection = hitTestRect.maxX(); > > + } else { > > + offsetStartInColumnDirection = width() - hitTestRect.maxX(); > > + offsetEndInColumnDirection = width() - hitTestRect.x(); > > + } > > } > > AFAICT this should use the table's style as we don't allow sections to have a writing mode different from the table. > I use the same style that was already in use. If that is wrong, then it was buggy already. > Also why is this not using RenderBox::flipForWritingMode? > There was a minor difference, but I can't remember what. I will take a look again. > > LayoutTests/fast/dom/nodesFromRect-table.html:85 > > + check(190, 175, 10, 20, 10, 20, [e.td22, e.td23, e.testtable]); > > + check(260, 275, 10, 20, 10, 20, [e.td43, e.td44, e.testtable]); > > + check(175, 190, 20, 10, 20, 10, [e.td22, e.td32, e.testtable]); > > + check(275, 260, 20, 10, 20, 10, [e.td34, e.td44, e.testtable]); > > + check(190, 190, 20, 20, 20, 20, [e.td22, e.td23, e.td32, e.td33, e.testtable]); > > Interestingly you don't get the rows, I am not 100% this is expected here as you have some spacing between your cells. I think you should define a table section by adding a <tbody id="tbody"> around your rows (here it's anonymous and maybe that's why you don't see it in your results). > We don't get the rows because rows are never returned in hit-tests. Even if you place spacing between cells, you will never be able to get a hit-test to hit a row. I know the history behind this, and I am not sure it makes sense anymore, but I don't want to change it in an unrelated patch. I considered adding the rows to the result if two cells from the same row are returned, but that might confuse touch-adjustment. Since it currently relies on being able to adjust the touch-point to a location where it hits the chosen target, that might be problematic and with rows being un-hittable, Thanks for the great comments. I will improve the ChangeLog, clean-up the test and improve coding style.

Allan Sandfeld Jensen

Comment 5 2012-05-29 04:32:27 PDT

Created attachment 144515 [details] Patch Cleaned-up and writing-mode direction tested

Allan Sandfeld Jensen

Comment 6 2012-05-29 04:38:39 PDT

(In reply to comment #3) > Btw, you do a binary search on the rows and *columns*, not cells. > I considered it a single 2D binary search, but I will correct the terminology if you think that is incorrect or confusing. > Also why is this not using RenderBox::flipForWritingMode? > I have used it for the first inversion now. The second seems to use slightly different test and a inverts over the last columnPos instead of width. To be honest I am not entirely sure why it doesn't use similar logic, so I am taking a cautious approach and keep the same logic.

Allan Sandfeld Jensen

Comment 7 2012-05-30 01:47:48 PDT

(In reply to comment #6) > (In reply to comment #3) > > Also why is this not using RenderBox::flipForWritingMode? > > > I have used it for the first inversion now. The second seems to use slightly different test and a inverts over the last columnPos instead of width. To be honest I am not entirely sure why it doesn't use similar logic, so I am taking a cautious approach and keep the same logic. The second flip is for text-direction not writing-mode. The two are unfortunately separate properties currently, and needs to be accounted for separately.

Allan Sandfeld Jensen

Comment 8 2012-05-30 01:55:27 PDT

Created attachment 144755 [details] Patch Add missing LayoutTest/ChangeLog

Julien Chaffraix

Comment 9 2012-05-30 11:10:40 PDT

Comment on attachment 144755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144755&action=review > Source/WebCore/rendering/RenderTableSection.cpp:1358 > + LayoutUnit offsetStartInColumnDirection; > + LayoutUnit offsetEndInColumnDirection; > + LayoutRect flippedHitTestRect = hitTestRect; > + flipForWritingMode(flippedHitTestRect); > + if (style()->isHorizontalWritingMode()) { > + offsetStartInColumnDirection = flippedHitTestRect.y(); > + offsetEndInColumnDirection = flippedHitTestRect.maxY(); > + } else { > + offsetStartInColumnDirection = flippedHitTestRect.x(); > + offsetEndInColumnDirection = flippedHitTestRect.maxX(); > + } It looks more messy than I would have expected but unfortunately makes total sense. I wonder if we couldn't re-use the logic from dirtiedRows / dirtiedColumns here. The reason being that it's already fully writing-mode aware (not direction aware though), already does the proper row / column patching and accounts for overflowing cell (which nodeAtPoint currently doesn't). It would also avoid code duplication. > LayoutTests/fast/dom/nodesFromRect-table.html:23 > + .rtl { > + direction: rtl; > + } > + .tblr { > + -webkit-writing-mode: vertical-lr; > + } AFAICT those 2 are unused and should be removed.

Allan Sandfeld Jensen

Comment 10 2012-05-30 12:53:33 PDT

(In reply to comment #9) > (From update of attachment 144755 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144755&action=review > > > Source/WebCore/rendering/RenderTableSection.cpp:1358 > > + LayoutUnit offsetStartInColumnDirection; > > + LayoutUnit offsetEndInColumnDirection; > > + LayoutRect flippedHitTestRect = hitTestRect; > > + flipForWritingMode(flippedHitTestRect); > > + if (style()->isHorizontalWritingMode()) { > > + offsetStartInColumnDirection = flippedHitTestRect.y(); > > + offsetEndInColumnDirection = flippedHitTestRect.maxY(); > > + } else { > > + offsetStartInColumnDirection = flippedHitTestRect.x(); > > + offsetEndInColumnDirection = flippedHitTestRect.maxX(); > > + } > > It looks more messy than I would have expected but unfortunately makes total sense. I wonder if we couldn't re-use the logic from dirtiedRows / dirtiedColumns here. It looks like that code does two binary lookups one for begin and one for end. Two lookups seems unnecessary here, and would make the common-case of point hit-testing slower. The code could be made cleaner by having an additional function to flip for direction, and first flip for writing-mode and then for direction, but this begs the unpleasant question: If writing mode is top-to-bottom and direction right-to-left, does the columns then flow bottom-to-top? and if that is the case, does that mean that if both writing-mode and direction is right-to-left that the columns flow left-to-right? > > > LayoutTests/fast/dom/nodesFromRect-table.html:23 > > + .rtl { > > + direction: rtl; > > + } > > + .tblr { > > + -webkit-writing-mode: vertical-lr; > > + } > > AFAICT those 2 are unused and should be removed. Right.

Julien Chaffraix

Comment 11 2012-05-30 13:45:33 PDT

Comment on attachment 144755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144755&action=review >>> Source/WebCore/rendering/RenderTableSection.cpp:1358 >>> + } >> >> It looks more messy than I would have expected but unfortunately makes total sense. I wonder if we couldn't re-use the logic from dirtiedRows / dirtiedColumns here. The reason being that it's already fully writing-mode aware (not direction aware though), already does the proper row / column patching and accounts for overflowing cell (which nodeAtPoint currently doesn't). It would also avoid code duplication. > > It looks like that code does two binary lookups one for begin and one for end. Two lookups seems unnecessary here, and would make the common-case of point hit-testing slower. > > The code could be made cleaner by having an additional function to flip for direction, and first flip for writing-mode and then for direction, but this begs the unpleasant question: If writing mode is top-to-bottom and direction right-to-left, does the columns then flow bottom-to-top? and if that is the case, does that mean that if both writing-mode and direction is right-to-left that the columns flow left-to-right? I don't really buy the performance argument. Those 2 functions have one big advantage: they have a nicer way to handle sparse overflowing cells. Currently it's handled above and is a quadratic algorithm. The 2nd binary search is still required, you just replaced it with checking explicitly when you have passed the extend of your rect below. For small rectangle testing, your explicit checks may win but the binary search gives us a better upper bound. On top of that, if you are afraid about performance, you could patch dirtiedRows / dirtiedColumns to avoid the second binary search for point hit-testing. We have some issues with 'direction' on table parts (especially when collapsing borders are involved). I don't understand your questions about writing mode vs direction. Those 2 properties handle orthogonal axis: writing mode defines the block flow direction (which translates how columns stack in a table), direction changes the inline flow direction (which translates how rows flow in a table). See CSS 3 writing mode for the details.

Allan Sandfeld Jensen

Comment 12 2012-05-31 01:43:09 PDT

(In reply to comment #11) > (From update of attachment 144755 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144755&action=review > > >>> Source/WebCore/rendering/RenderTableSection.cpp:1358 > >>> + } > >> > >> It looks more messy than I would have expected but unfortunately makes total sense. I wonder if we couldn't re-use the logic from dirtiedRows / dirtiedColumns here. The reason being that it's already fully writing-mode aware (not direction aware though), already does the proper row / column patching and accounts for overflowing cell (which nodeAtPoint currently doesn't). It would also avoid code duplication. > > > > It looks like that code does two binary lookups one for begin and one for end. Two lookups seems unnecessary here, and would make the common-case of point hit-testing slower. > > > > The code could be made cleaner by having an additional function to flip for direction, and first flip for writing-mode and then for direction, but this begs the unpleasant question: If writing mode is top-to-bottom and direction right-to-left, does the columns then flow bottom-to-top? and if that is the case, does that mean that if both writing-mode and direction is right-to-left that the columns flow left-to-right? > > I don't really buy the performance argument. Those 2 functions have one big advantage: they have a nicer way to handle sparse overflowing cells. Currently it's handled above and is a quadratic algorithm. The 2nd binary search is still required, you just replaced it with checking explicitly when you have passed the extend of your rect below. For small rectangle testing, your explicit checks may win but the binary search gives us a better upper bound. On top of that, if you are afraid about performance, you could patch dirtiedRows / dirtiedColumns to avoid the second binary search for point hit-testing. > No, in this case we are not looking for the start and the end, we are looking for all cells in between and need to visit all the cells in between. The linear search from the start is not a sub-optimal solution in this case, since we need to linearly walk all those cells anyway. This makes a second binary look-up a complete waste.

Julien Chaffraix

Comment 13 2012-05-31 06:33:07 PDT

Comment on attachment 144755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144755&action=review >>>>> Source/WebCore/rendering/RenderTableSection.cpp:1358 >>>>> + } >>>> >>>> It looks more messy than I would have expected but unfortunately makes total sense. I wonder if we couldn't re-use the logic from dirtiedRows / dirtiedColumns here. The reason being that it's already fully writing-mode aware (not direction aware though), already does the proper row / column patching and accounts for overflowing cell (which nodeAtPoint currently doesn't). It would also avoid code duplication. >>> >>> It looks like that code does two binary lookups one for begin and one for end. Two lookups seems unnecessary here, and would make the common-case of point hit-testing slower. >>> >>> The code could be made cleaner by having an additional function to flip for direction, and first flip for writing-mode and then for direction, but this begs the unpleasant question: If writing mode is top-to-bottom and direction right-to-left, does the columns then flow bottom-to-top? and if that is the case, does that mean that if both writing-mode and direction is right-to-left that the columns flow left-to-right? >> >> I don't really buy the performance argument. Those 2 functions have one big advantage: they have a nicer way to handle sparse overflowing cells. Currently it's handled above and is a quadratic algorithm. The 2nd binary search is still required, you just replaced it with checking explicitly when you have passed the extend of your rect below. For small rectangle testing, your explicit checks may win but the binary search gives us a better upper bound. On top of that, if you are afraid about performance, you could patch dirtiedRows / dirtiedColumns to avoid the second binary search for point hit-testing. >> >> We have some issues with 'direction' on table parts (especially when collapsing borders are involved). I don't understand your questions about writing mode vs direction. Those 2 properties handle orthogonal axis: writing mode defines the block flow direction (which translates how columns stack in a table), direction changes the inline flow direction (which translates how rows flow in a table). See CSS 3 writing mode for the details. > > No, in this case we are not looking for the start and the end, we are looking for all cells in between and need to visit all the cells in between. The linear search from the start is not a sub-optimal solution in this case, since we need to linearly walk all those cells anyway. This makes a second binary look-up a complete waste. Maybe I am missing something here, please tell me if that's the case. My point was not about avoiding the linear walk as it's needed to collect the cells. The issue was that the new code checks *every* cell to know if they are in the hit rect. Doing a binary search upfront removes the need for this check and *may* be better (the binary search may end up doing less comparisons, especially for bigger hit rect). If we can't say that our hit rect will be always small, the binary search with a tweak for the hit point case seemed like a better trade-off.

Antonio Gomes

Comment 14 2012-05-31 06:35:46 PDT

they we did internally for the blackberry port, is that we make the hit test rect a region, and as we walk over the cells, we remove the rect that intersects to the current cell from the rect. hit testing is not needed when the region becomes empty.

Allan Sandfeld Jensen

Comment 15 2012-05-31 07:08:50 PDT

(In reply to comment #13) > > Maybe I am missing something here, please tell me if that's the case. My point was not about avoiding the linear walk as it's needed to collect the cells. The issue was that the new code checks *every* cell to know if they are in the hit rect. Doing a binary search upfront removes the need for this check and *may* be better (the binary search may end up doing less comparisons, especially for bigger hit rect). If we can't say that our hit rect will be always small, the binary search with a tweak for the hit point case seemed like a better trade-off. I see where you are going, but it is unfortunately necessary to check every cell, since we do not only want the table-cells in the hit-test area, but all elements in the area, so we need to also check and add the content of each cell to the output. It is not just walking them, it is a tree recursion down each cell.

Antonio Gomes

Comment 16 2012-05-31 07:10:19 PDT

(In reply to comment #15) > (In reply to comment #13) > > > > Maybe I am missing something here, please tell me if that's the case. My point was not about avoiding the linear walk as it's needed to collect the cells. The issue was that the new code checks *every* cell to know if they are in the hit rect. Doing a binary search upfront removes the need for this check and *may* be better (the binary search may end up doing less comparisons, especially for bigger hit rect). If we can't say that our hit rect will be always small, the binary search with a tweak for the hit point case seemed like a better trade-off. > > I see where you are going, but it is unfortunately necessary to check every cell, since we do not only want the table-cells in the hit-test area, but all elements in the area, so we need to also check and add the content of each cell to the output. It is not just walking them, it is a tree recursion down each cell. not sure I agree. see my comment above.

Allan Sandfeld Jensen

Comment 17 2012-05-31 07:37:23 PDT

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #13) > > > > > > Maybe I am missing something here, please tell me if that's the case. My point was not about avoiding the linear walk as it's needed to collect the cells. The issue was that the new code checks *every* cell to know if they are in the hit rect. Doing a binary search upfront removes the need for this check and *may* be better (the binary search may end up doing less comparisons, especially for bigger hit rect). If we can't say that our hit rect will be always small, the binary search with a tweak for the hit point case seemed like a better trade-off. > > > > I see where you are going, but it is unfortunately necessary to check every cell, since we do not only want the table-cells in the hit-test area, but all elements in the area, so we need to also check and add the content of each cell to the output. It is not just walking them, it is a tree recursion down each cell. > > not sure I agree. see my comment above. Maybe I have misunderstood you, but you seem to be talking about an optimisation for overlapping elements. This particular patch is fixing the fast-path where no cells overlap each other. We could still do an optimisation like what you describe for the slow path, but the slow path is currently correct (but slow), where this patch fixes the fast path which is currently now even handling area-based hit-testing.

Allan Sandfeld Jensen

Comment 18 2012-06-01 01:55:18 PDT

Created attachment 145243 [details] Patch

Allan Sandfeld Jensen

Comment 19 2012-06-01 01:58:28 PDT

(In reply to comment #18) > Created an attachment (id=145243) [details] > Patch The new patch introduces some new helper function that can probably also be useful to, or be used to replace dirtiedRows and dirtiedColumns. Doing that might even fix various problems with right-to-left support in repainting.

Julien Chaffraix

Comment 20 2012-06-04 10:36:39 PDT

Comment on attachment 145243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145243&action=review > Source/WebCore/rendering/RenderTableSection.cpp:1112 > + unsigned nextRow = std::upper_bound(m_rowPos.begin(), m_rowPos.end(), flippedRect.y()) - m_rowPos.begin(); Shouldn't using upper_bound cause some issues as it would happily ignore 0px height rows? (for visual hit testing, it may not be an issue but for programmatic testing, I would expect those rows to be considered). > Source/WebCore/rendering/RenderTableSection.cpp:1115 > + return CellSpan(m_rowPos.size(), m_rowPos.size()); // No rows spanned. It may be worth thinking about having some CellSpan::emptySpan()? (this is kind of a magic constant) > Source/WebCore/rendering/RenderTableSection.h:213 > + // These two functions take a rectangle as input that has been flipped by flipRectToRowAndColumnDirection. > + CellSpan spannedRows(const LayoutRect& flippedRect) const; > + CellSpan spannedColumns(const LayoutRect& flippedRect) const; Why do we need more functions that are doing exactly the same work as dirtiedRows and dirtiedColumns? Nowhere can I find why this is preferable to patching / re-using the above functions.

Allan Sandfeld Jensen

Comment 21 2012-06-04 10:58:54 PDT

(In reply to comment #20) > (From update of attachment 145243 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145243&action=review > > > Source/WebCore/rendering/RenderTableSection.cpp:1112 > > + unsigned nextRow = std::upper_bound(m_rowPos.begin(), m_rowPos.end(), flippedRect.y()) - m_rowPos.begin(); > > Shouldn't using upper_bound cause some issues as it would happily ignore 0px height rows? (for visual hit testing, it may not be an issue but for programmatic testing, I would expect those rows to be considered). > I used upper bound because the existing test used upper bound. I wanted it to have similar behaviour for point-based testing. > > Source/WebCore/rendering/RenderTableSection.cpp:1115 > > + return CellSpan(m_rowPos.size(), m_rowPos.size()); // No rows spanned. > > It may be worth thinking about having some CellSpan::emptySpan()? (this is kind of a magic constant) > The constant is not magic, the span is end-exclusive and any span with start=end is empty. This return value includes the information that the hit area is after all rows, similar (0,0) will be be returned for hit areas before all rows. This is used in the patch mentioned below. > > Source/WebCore/rendering/RenderTableSection.h:213 > > + // These two functions take a rectangle as input that has been flipped by flipRectToRowAndColumnDirection. > > + CellSpan spannedRows(const LayoutRect& flippedRect) const; > > + CellSpan spannedColumns(const LayoutRect& flippedRect) const; > > Why do we need more functions that are doing exactly the same work as dirtiedRows and dirtiedColumns? Nowhere can I find why this is preferable to patching / re-using the above functions. Yes I agree :) But I have patched that separately because reusing these functions slightly changes dirty behaviour (fixing separate bugs), and might have separate issues, see https://bugs.webkit.org/show_bug.cgi?id=88066

Julien Chaffraix

Comment 22 2012-06-05 07:56:26 PDT

Comment on attachment 145243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145243&action=review > Source/WebCore/rendering/RenderTableSection.cpp:1093 > +LayoutRect RenderTableSection::flipRectToRowAndColumnDirection(const LayoutRect& rect) const Not a huge fan of the naming here. First 'direction' only apply in the "row" direction (which is technically the inline base direction). Also you transpose the rectangle to align it with the writing mode coordinates - which is uncommon in the code, (x, y) being physical coordinates - the naming should use 'logical' as it's not a normal rect. Better suggestions on naming: logicalRectForWritingModeAndDirection(), flippedLogicalRectForWritingModeAndDirection() >>> Source/WebCore/rendering/RenderTableSection.cpp:1112 >>> + unsigned nextRow = std::upper_bound(m_rowPos.begin(), m_rowPos.end(), flippedRect.y()) - m_rowPos.begin(); >> >> Shouldn't using upper_bound cause some issues as it would happily ignore 0px height rows? (for visual hit testing, it may not be an issue but for programmatic testing, I would expect those rows to be considered). > > I used upper bound because the existing test used upper bound. I wanted it to have similar behaviour for point-based testing. Please add a FIXME in this case. This is kind of a corner cases but it should be tested. >>> Source/WebCore/rendering/RenderTableSection.cpp:1115 >>> + return CellSpan(m_rowPos.size(), m_rowPos.size()); // No rows spanned. >> >> It may be worth thinking about having some CellSpan::emptySpan()? (this is kind of a magic constant) > > The constant is not magic, the span is end-exclusive and any span with start=end is empty. This return value includes the information that the hit area is after all rows, similar (0,0) will be be returned for hit areas before all rows. This is used in the patch mentioned below. Note that considering the end is a departure from the other usages of CellSpan. I don't really care either way but we need to be consistent. >>> Source/WebCore/rendering/RenderTableSection.h:213 >>> + CellSpan spannedColumns(const LayoutRect& flippedRect) const; >> >> Why do we need more functions that are doing exactly the same work as dirtiedRows and dirtiedColumns? Nowhere can I find why this is preferable to patching / re-using the above functions. > > Yes I agree :) But I have patched that separately because reusing these functions slightly changes dirty behaviour (fixing separate bugs), and might have separate issues, see https://bugs.webkit.org/show_bug.cgi?id=88066 ACK, I missed the bug. Adding a huge FIXME here mentioning bug 88066 would help people not following closely what is going on.

Allan Sandfeld Jensen

Comment 23 2012-06-06 00:56:37 PDT

(In reply to comment #22) > > The constant is not magic, the span is end-exclusive and any span with start=end is empty. This return value includes the information that the hit area is after all rows, similar (0,0) will be be returned for hit areas before all rows. This is used in the patch mentioned below. > > Note that considering the end is a departure from the other usages of CellSpan. I don't really care either way but we need to be consistent. > Well, it is consistent with how rows and columns vectors are accessed, and also the values returned by fullTableColumnSpan for instance. The row and columns vectors are always one larger than the number of rows and columns, with the last value representing the end of the last row or column. m_rowPos[0] == beginning of first row m_rowPos[m_rowPos.size()-1] == end of the last row. I will add a comment making it more explicit.

Allan Sandfeld Jensen

Comment 24 2012-06-06 01:54:08 PDT

Created attachment 145964 [details] Patch

Julien Chaffraix

Comment 25 2012-06-08 11:44:06 PDT

Comment on attachment 145964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145964&action=review r=me > Source/WebCore/rendering/RenderTableSection.cpp:1112 > + // FIXME: We should reevaluate if upper_bound is the correct choice here. A more precise comment would help. The issue is that upper_bound will ignore empty rows. It may be fine for painting, not so much for hit testing. Also currently rows / columns are handled slightly differently (upper_bound vs lower_bound). I don't see why it should be different but it already was so that's actually another FIXME :) > Source/WebCore/rendering/RenderTableSection.h:226 > + // The returned span of rows or columns is end-exclusive, and empty if start==end. Terrific, thanks for documenting that.

Allan Sandfeld Jensen

Comment 26 2012-06-10 08:44:39 PDT

Created attachment 146739 [details] Patch for landing

WebKit Review Bot

Comment 27 2012-06-11 02:39:04 PDT

Comment on attachment 146739 [details] Patch for landing Clearing flags on attachment: 146739 Committed r119967: <http://trac.webkit.org/changeset/119967>

WebKit Review Bot

Comment 28 2012-06-11 02:39:10 PDT

All reviewed patches have been landed. Closing bug.

Attachments
Patch (12.85 KB, patch) 2012-05-16 03:39 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (12.74 KB, patch) 2012-05-25 05:09 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (13.32 KB, patch) 2012-05-29 04:32 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (13.80 KB, patch) 2012-05-30 01:55 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (16.02 KB, patch) 2012-06-01 01:55 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch (17.16 KB, patch) 2012-06-06 01:54 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Patch for landing (17.27 KB, patch) 2012-06-10 08:44 PDT, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Bug 86605