| Summary: | There is additional space not belonged to a table between the table cells | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Vladimir <vladimirvf> | ||||||||||||||||||||||||||
| Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | allan.jensen, arpitabahuguna, darin, dglazkov, eae, eric, ivpolyakov, jchaffraix, leviw, robert, webkit.review.bot | ||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | HasReduction | ||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
Created attachment 120170 [details]
Html page with to reproduce the issue and a video that demonstrates the issue
Created attachment 132491 [details]
Inline repro (from the previous test attachment) - hover over the middle border to see the bug
Confirmed on ToT.
It looks like an edge issue on our hit testing code.
*** Bug 90181 has been marked as a duplicate of this bug. *** Created attachment 156530 [details]
Patch
Good catch, but does this solve the same case for a table with cellspacing? I guess this code will send the hit-test on to test the next cell, but the hit-test will still not be caught because it not actually inside the border-box of the next cell? Perhaps both cases should actually hit the row or table-section instead? Btw, i think the test-cases could be a little simpler if they used document.elementAtPoint instead of relying on sending and catching events. (In reply to comment #5) > Good catch, but does this solve the same case for a table with cellspacing? I guess this code will send the hit-test on to test the next cell, but the hit-test will still not be caught because it not actually inside the border-box of the next cell? Perhaps both cases should actually hit the row or table-section instead? > Hi Allan, thank-you for your comment and apologies for the delayed response. You are right in assuming that this change does not fix the issue when a table with cellspacing is specified. My fix is simply for the first pixel of the column not being recognized by our hittest logic. I can perhaps raise another bug for the cellspacing scenario and work on it separately? > Btw, i think the test-cases could be a little simpler if they used document.elementAtPoint instead of relying on sending and catching events. I shall upload another patch by using document.elementAtPoint instead; thanks for pointing it out. :) Created attachment 156637 [details]
Patch
Comment on attachment 156530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156530&action=review > Source/WebCore/rendering/RenderTableSection.cpp:1121 > // Find the first columnt that starts after rect left. Btw, feel free to fix my typo. (In reply to comment #7) > Created an attachment (id=156637) [details] > Patch I can't provide an r+ as a non-reviewer, but it looks good to me now and the test-case is very informative. Created attachment 156642 [details]
Patch
(In reply to comment #9) > (In reply to comment #7) > > Created an attachment (id=156637) [details] [details] > > Patch > > I can't provide an r+ as a non-reviewer, but it looks good to me now and the test-case is very informative. (In reply to comment #8) > (From update of attachment 156530 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156530&action=review > > > Source/WebCore/rendering/RenderTableSection.cpp:1121 > > // Find the first columnt that starts after rect left. > > Btw, feel free to fix my typo. Done! and thanks for your positive feedback. :) Also, I was looking at perhaps raising another bug which removes the following FIXME from the code (in RenderTableSection::spannedRows()): // FIXME: Upper_bound might not be the correct algorithm here since it might skip empty rows, but it is // consistent with behavior in the former point based hit-testing (but inconsistent with spannedColumns). I could add testcases with empty <TR>s to verify upper_bound() algorithm's behavior. Would appreciate your thoughts on the same. > (In reply to comment #8)
> Also, I was looking at perhaps raising another bug which removes the following FIXME from the code (in RenderTableSection::spannedRows()):
>
> // FIXME: Upper_bound might not be the correct algorithm here since it might skip empty rows, but it is
> // consistent with behavior in the former point based hit-testing (but inconsistent with spannedColumns).
>
> I could add testcases with empty <TR>s to verify upper_bound() algorithm's behavior.
Make a test-case verifying or invalidating the problem would be great.
For this patch though, a guess the comment actually needs to be updated to remove the claim that it is inconsistant with spannedColumns, since you changed that.
But talking about what happens with empty-cells made me take another look at your empty-cell test-case. Since 'empty-cells' default to 'show' which means td3 and td4 renders their own border, doesn't your test-case actually demonstrating a bug when it says elementAtPoint on the borders of td3 hits td5? And a really big bug when hitting the td3 border from td4 suddenly hits td2?
(In reply to comment #12) > But talking about what happens with empty-cells made me take another look at your empty-cell test-case. Since 'empty-cells' default to 'show' which means td3 and td4 renders their own border, doesn't your test-case actually demonstrating a bug when it says elementAtPoint on the borders of td3 hits td5? And a really big bug when hitting the td3 border from td4 suddenly hits td2? Okay, so there are no borders on the table which is why it is correct. I guess that demonstrates the concern we raised about collapsed rows works correctly with collapsed cells. (In reply to comment #12) > > (In reply to comment #8) > > Also, I was looking at perhaps raising another bug which removes the following FIXME from the code (in RenderTableSection::spannedRows()): > > > > // FIXME: Upper_bound might not be the correct algorithm here since it might skip empty rows, but it is > > // consistent with behavior in the former point based hit-testing (but inconsistent with spannedColumns). > > > > I could add testcases with empty <TR>s to verify upper_bound() algorithm's behavior. > > Make a test-case verifying or invalidating the problem would be great. > > For this patch though, a guess the comment actually needs to be updated to remove the claim that it is inconsistant with spannedColumns, since you changed that. > I agree, shall take care of updating this comment as well. > But talking about what happens with empty-cells made me take another look at your empty-cell test-case. Since 'empty-cells' default to 'show' which means td3 and td4 renders their own border, doesn't your test-case actually demonstrating a bug when it says elementAtPoint on the borders of td3 hits td5? And a really big bug when hitting the td3 border from td4 suddenly hits td2? > > Okay, so there are no borders on the table which is why it is correct. I guess that demonstrates the concern we raised about collapsed rows works correctly with collapsed cells. > I verified the empty-columns testcase, modifying it by adding borders for all table-cells. Even in that case our behavior remains consistent with that of other browsers (Opera, FF). The o/p on all browsers was the following: Executing for element: td1 Node at edge-1: td1 Node at edge: Node at edge+1: Executing for element: td2 Node at edge-1: td2 Node at edge: Node at edge+1: Executing for element: td3 Node at edge-1: td3 Node at edge: Node at edge+1: Executing for element: td4 Node at edge-1: td4 Node at edge: Node at edge+1: Executing for element: td5 Node at edge-1: td5 Node at edge: Node at edge+1: So I suppose the change to upper_bound() does not affect in the spannedColumns() case. I need to verify for spannedRows() though. Comment on attachment 156642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156642&action=review hittest-tablecell-edge.html is a subset of hittest-tablecell-edge-empty-columns.html. Do we really need those 2 cases or would hittest-tablecell-edge-empty-columns.html be enough? I would support landing a test with borders on the table-cells similar to hittest-tablecell-edge-empty-columns.html. Also, it would be nice to do testing on several rows (it shouldn't be impacted by your change but that should be easy to add as part of your change). Due to the tabs, this change will need another round but it looks good. > Source/WebCore/rendering/RenderTableSection.cpp:1122 > + unsigned nextColumn = std::upper_bound(columnPos.begin(), columnPos.end(), flippedRect.x()) - columnPos.begin(); Please amend the FIXME in spannedRows as it's not true anymore. Don't forget to mention that this matches other browsers. > LayoutTests/fast/dom/hittest-tablecell-edge.html:21 > + ['td1', 'td2', 'td3'].forEach(function(a) { > + ele[a] = document.getElementById(a); > + }); > + > + ['td1', 'td2', 'td3'].forEach(function(a) { > + hittest(ele[a], a); > + }); > + > + ['td1', 'td2', 'td3'].forEach(function(a) { > + ele[a].innerHTML = ''; > + }); We can maybe fold all those forEach into one? > LayoutTests/fast/dom/hittest-tablecell-edge.html:32 > + y = ele.getBoundingClientRect().bottom/2; > + debug('Executing for element: '+orgElement); > + debug('Node at edge-1: '+document.elementFromPoint(x-1,y).id); > + debug('Node at edge: '+document.elementFromPoint(x,y).id); > + debug('Node at edge+1: '+document.elementFromPoint(x+1,y).id); It's prefered to use WebKit style for tests. > LayoutTests/fast/dom/hittest-tablecell-edge.html:51 > + <td id="td3">1</td> Those are tabs, your patch will be rejected by our commit-hook. Created attachment 156934 [details]
Patch
(In reply to comment #15) > (From update of attachment 156642 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156642&action=review Thank-you for the review Julien. Have uploaded another patch as per your comments. > > hittest-tablecell-edge.html is a subset of hittest-tablecell-edge-empty-columns.html. Do we really need those 2 cases or would hittest-tablecell-edge-empty-columns.html be enough? > > I would support landing a test with borders on the table-cells similar to hittest-tablecell-edge-empty-columns.html. > > Also, it would be nice to do testing on several rows (it shouldn't be impacted by your change but that should be easy to add as part of your change). Have now added 4 testcases, all containing empty table cells and multiple rows, that carry out the hittest on the edge of adjoining table cells, both on the right edge as well as on the bottom edge (this has been added just to verify the rows behavior). Also, have added separate testcases that have borders specified for the table-cells to highlight the difference in the hittesting results on the edge, with and without borders. Please note that for all these cases, our behavior is now exactly similar to that of Opera and Firefox. > Please amend the FIXME in spannedRows as it's not true anymore. Don't forget to mention that this matches other browsers. > Have removed the FIXME in spannedRows and also added a comment in spannedColumns. > > LayoutTests/fast/dom/hittest-tablecell-edge.html:21 > > + ['td1', 'td2', 'td3'].forEach(function(a) { > > + ele[a] = document.getElementById(a); > > + }); > > + > > + ['td1', 'td2', 'td3'].forEach(function(a) { > > + hittest(ele[a], a); > > + }); > > + > > + ['td1', 'td2', 'td3'].forEach(function(a) { > > + ele[a].innerHTML = ''; > > + }); > > We can maybe fold all those forEach into one? > Done. > > LayoutTests/fast/dom/hittest-tablecell-edge.html:32 > > + y = ele.getBoundingClientRect().bottom/2; > > + debug('Executing for element: '+orgElement); > > + debug('Node at edge-1: '+document.elementFromPoint(x-1,y).id); > > + debug('Node at edge: '+document.elementFromPoint(x,y).id); > > + debug('Node at edge+1: '+document.elementFromPoint(x+1,y).id); > > It's prefered to use WebKit style for tests. > Changed these to use shouldBe() instead. > > LayoutTests/fast/dom/hittest-tablecell-edge.html:51 > > + <td id="td3">1</td> > > Those are tabs, your patch will be rejected by our commit-hook. I apologize for this oversight. Shall take care of this in the future. (In reply to comment #17) > (In reply to comment #15) > > > LayoutTests/fast/dom/hittest-tablecell-edge.html:32 > > > + y = ele.getBoundingClientRect().bottom/2; > > > + debug('Executing for element: '+orgElement); > > > + debug('Node at edge-1: '+document.elementFromPoint(x-1,y).id); > > > + debug('Node at edge: '+document.elementFromPoint(x,y).id); > > > + debug('Node at edge+1: '+document.elementFromPoint(x+1,y).id); > > > > It's prefered to use WebKit style for tests. > > > Changed these to use shouldBe() instead. > I think he meant also coding style. The style-checker doesn't enforce WebKit coding style in javascript, but it is a good idea to use it where it make sense, like adding a space after comma or around binary operators. Comment on attachment 156934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156934&action=review Allan was right, sorry for being unclear. > Source/WebCore/rendering/RenderTableSection.cpp:1121 > + // The use of upper_bound algorithm makes the behavior of hittest (on table-cell edges) in accordance > + // with other browsers. I think you should substantiate on the 'why' we match other browsers (it's good to show that you tested it and found us in compliance but it's a bit dry). How is upper_bound better than lower_bound here? This would be a high value comment for anyone reading this code. > Source/WebCore/rendering/RenderTableSection.cpp:1122 > + // https://bugs.webkit.org/show_bug.cgi?id=74864 I don't think adding the bug number adds any information to the change. My rule of thumb for including a bug number is either to track an existing failure or because the bug has a discussion that adds a lot to the understanding of how the code is structured or why it behaves this way. > LayoutTests/fast/dom/hittest-tablecell-bottom-edge.html:24 > + x = ele.getBoundingClientRect().right/2; Nit: middleX may be more readable in the output. (not repeated for the other tests) Created attachment 157438 [details]
Patch
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #15) > > > > LayoutTests/fast/dom/hittest-tablecell-edge.html:32 > > > > + y = ele.getBoundingClientRect().bottom/2; > > > > + debug('Executing for element: '+orgElement); > > > > + debug('Node at edge-1: '+document.elementFromPoint(x-1,y).id); > > > > + debug('Node at edge: '+document.elementFromPoint(x,y).id); > > > > + debug('Node at edge+1: '+document.elementFromPoint(x+1,y).id); > > > > > > It's prefered to use WebKit style for tests. > > > > > Changed these to use shouldBe() instead. > > > I think he meant also coding style. The style-checker doesn't enforce WebKit coding style in javascript, but it is a good idea to use it where it make sense, like adding a space after comma or around binary operators. Thanks for the clarification Allan. :) (In reply to comment #19) > (From update of attachment 156934 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156934&action=review > > Allan was right, sorry for being unclear. > Ah! I completely misunderstood. Have made the necessary changes now. Shall also take care of this in the future. > > Source/WebCore/rendering/RenderTableSection.cpp:1121 > > + // The use of upper_bound algorithm makes the behavior of hittest (on table-cell edges) in accordance > > + // with other browsers. > > I think you should substantiate on the 'why' we match other browsers (it's good to show that you tested it and found us in compliance but it's a bit dry). How is upper_bound better than lower_bound here? This would be a high value comment for anyone reading this code. > Have tried to add an explanatory comment as to why upper_bound is preferred over lower_bound. > > Source/WebCore/rendering/RenderTableSection.cpp:1122 > > + // https://bugs.webkit.org/show_bug.cgi?id=74864 > > I don't think adding the bug number adds any information to the change. My rule of thumb for including a bug number is either to track an existing failure or because the bug has a discussion that adds a lot to the understanding of how the code is structured or why it behaves this way. > Understand. Have removed the same. > > LayoutTests/fast/dom/hittest-tablecell-bottom-edge.html:24 > > + x = ele.getBoundingClientRect().right/2; > > Nit: middleX may be more readable in the output. (not repeated for the other tests) Made the required changes. Comment on attachment 157438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157438&action=review > Source/WebCore/ChangeLog:17 > + fast/dom/hittest-tablecell-with-borders-right-edge.html I didn't notice that those tests were in fast/dom. It doesn't make sense to put them there - you are not testing a dom API - and they would be better with the other table tests in fast/tables. If you prefer you could even put them in a sub-directory (fast/table/testing). > Source/WebCore/rendering/RenderTableSection.cpp:1129 > + // of other browsers. Woua, that's a long explanation! It's also half true as spanningRows / spanningColumns are used for hit testing but also painting. Let's try to shorten the comment (feel free to adapt): // lower_bound doesn't handle the edge between 2 cells properly as it would wrongly return the cell on the logical top / left. // upper_bound properly return the cell on the logical bottom / right, which matches other browsers. > LayoutTests/fast/dom/hittest-tablecell-bottom-edge-expected.txt:41 > +PASS document.elementFromPoint(middleX, edge - 1).id is '' This result is interesting. Note that this means that there is a 1px gap (or more) around an empty rows. This matches the empty column case too. As this patch makes us only progress, I am in favor of filing a follow-up bug about that. > LayoutTests/fast/dom/hittest-tablecell-with-borders-right-edge-expected.txt:47 > +PASS document.elementFromPoint(edge - 1, middleY).id is '' > +PASS document.elementFromPoint(edge, middleY).id is '' > +PASS document.elementFromPoint(edge + 1, middleY).id is '' I don't think this is fine to return nothing in all those cases but you are just validating our existing behavior here. Created attachment 157860 [details]
Patch
(In reply to comment #23) > (From update of attachment 157438 [details]) Thanks for the review Julien. > View in context: https://bugs.webkit.org/attachment.cgi?id=157438&action=review > > > Source/WebCore/ChangeLog:17 > > + fast/dom/hittest-tablecell-with-borders-right-edge.html > > I didn't notice that those tests were in fast/dom. It doesn't make sense to put them there - you are not testing a dom API - and they would be better with the other table tests in fast/tables. If you prefer you could even put them in a sub-directory (fast/table/testing). > > > Source/WebCore/rendering/RenderTableSection.cpp:1129 > > + // of other browsers. > Have added the testcases under fast/table/testing now. > Woua, that's a long explanation! It's also half true as spanningRows / spanningColumns are used for hit testing but also painting. > > Let's try to shorten the comment (feel free to adapt): > // lower_bound doesn't handle the edge between 2 cells properly as it would wrongly return the cell on the logical top / left. > // upper_bound properly return the cell on the logical bottom / right, which matches other browsers. > Made the change. > > LayoutTests/fast/dom/hittest-tablecell-bottom-edge-expected.txt:41 > > +PASS document.elementFromPoint(middleX, edge - 1).id is '' > > This result is interesting. Note that this means that there is a 1px gap (or more) around an empty rows. This matches the empty column case too. As this patch makes us only progress, I am in favor of filing a follow-up bug about that. > Yes, that's true. I shall post a bug for the same and update. Also, currently FF seems to be the only browser returning a TableRow element for a hit test on a cellspacing area. Other browsers including WebKit do not return any underlying element. Should that be treated as a bug? Are we expected to return the TableRow element in such a case. If so, I shall raise another bug for it as well. > > LayoutTests/fast/dom/hittest-tablecell-with-borders-right-edge-expected.txt:47 > > +PASS document.elementFromPoint(edge - 1, middleY).id is '' > > +PASS document.elementFromPoint(edge, middleY).id is '' > > +PASS document.elementFromPoint(edge + 1, middleY).id is '' > > I don't think this is fine to return nothing in all those cases but you are just validating our existing behavior here. Am uncertain as to what changes, if required, are to be made for these cases. Did you perhaps wish for these cases to be removed? Comment on attachment 157860 [details] Patch Attachment 157860 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13473640 New failing tests: fast/table/testing/hittest-tablecell-with-borders-right-edge.html fast/table/testing/hittest-tablecell-bottom-edge.html fast/table/testing/hittest-tablecell-right-edge.html fast/table/testing/hittest-tablecell-with-borders-bottom-edge.html Created attachment 157861 [details]
Archive of layout-test-results from gce-cr-linux-05
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 157865 [details]
Patch
(In reply to comment #28) > Created an attachment (id=157865) [details] > Patch Once you have gotten an r+, you don't have to ask for it again. Just upload the patch with a Changelog that already has the reviewer filled out. (In reply to comment #29) > (In reply to comment #28) > > Created an attachment (id=157865) [details] [details] > > Patch > > Once you have gotten an r+, you don't have to ask for it again. Just upload the patch with a Changelog that already has the reviewer filled out. Oh! okay. I wasn't aware of that. Thanks for the info. Shall keep that in mind henceforth. Comment on attachment 157865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157865&action=review > Source/WebCore/ChangeLog:17 > + fast/table/testing/hittest-tablecell-with-borders-right-edge.html testing is too generic and is misleading (how can a test not be testing). hit-testing is what you want if you think you need a subdirectory. Alternatively you can just put it directly under fast/table. Created attachment 158215 [details]
Patch
(In reply to comment #31) > (From update of attachment 157865 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157865&action=review > > > Source/WebCore/ChangeLog:17 > > + fast/table/testing/hittest-tablecell-with-borders-right-edge.html > > testing is too generic and is misleading (how can a test not be testing). hit-testing is what you want if you think you need a subdirectory. Alternatively you can just put it directly under fast/table. Have uploaded another patch Julien, moving the testcases directly under fast/table. Perhaps I misunderstood your previous review comment regarding moving them to a new sub-directory (fast/table/testing). Apologize for the same. Comment on attachment 158215 [details]
Patch
Great, sorry for being unclear in my comment (I was thinking of fast/table/hit-testing or just fast/table).
Comment on attachment 158215 [details] Patch Clearing flags on attachment: 158215 Committed r125684: <http://trac.webkit.org/changeset/125684> All reviewed patches have been landed. Closing bug. (In reply to comment #34) > (From update of attachment 158215 [details]) > Great, sorry for being unclear in my comment (I was thinking of fast/table/hit-testing or just fast/table). Thank-you for your time and the review Julien, Allan. |
Created attachment 119864 [details] Obsolete attachment If a mouse cursor hovers between cells of the following table: <head> <title></title> <style> td { padding: 5px; cursor: pointer; border: solid 2px black; } </style> </head> <body> <table cellpadding="0" cellspacing="0" border="0"> <tr> <td>1</td> <td>2</td> </tr> </table> </body> </html> the cursor is not considered as hovered over the table (see the attached video).