Bug 74864

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:

Keywords:

HasReduction

Version:

528+ (Nightly build)

Hardware:

OS:

Unspecified

Attachments:

Description	Flags
Obsolete attachment	none
Html page with to reproduce the issue and a video that demonstrates the issue	none
Inline repro (from the previous test attachment) - hover over the middle border to see the bug	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from gce-cr-linux-05	none
Patch	none
Patch	none

Vladimir

Reported 2011-12-19 07:59:54 PST

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).

Attachments
Obsolete attachment (1.59 MB, application/x-zip-compressed) 2011-12-19 07:59 PST, Vladimir	no flags	Details
Html page with to reproduce the issue and a video that demonstrates the issue (440.43 KB, application/x-zip-compressed) 2011-12-21 04:21 PST, Vladimir	no flags	Details
Inline repro (from the previous test attachment) - hover over the middle border to see the bug (447 bytes, text/html) 2012-03-18 08:23 PDT, Julien Chaffraix	no flags	Details
Patch (10.80 KB, patch) 2012-08-04 04:56 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (9.62 KB, text/plain) 2012-08-06 02:42 PDT, Arpita Bahuguna	no flags	Details
Patch (9.56 KB, patch) 2012-08-06 03:01 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (47.33 KB, patch) 2012-08-07 07:17 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (48.55 KB, patch) 2012-08-09 05:14 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (48.59 KB, patch) 2012-08-11 01:57 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-05 (610.96 KB, application/zip) 2012-08-11 02:33 PDT, WebKit Review Bot	no flags	Details
Patch (48.50 KB, patch) 2012-08-11 03:19 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (47.97 KB, patch) 2012-08-13 22:23 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Vladimir

Comment 1 2011-12-21 04:21:12 PST

Created attachment 120170 [details] Html page with to reproduce the issue and a video that demonstrates the issue

Julien Chaffraix

Comment 2 2012-03-18 08:23:01 PDT

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.

Tony Chang

Comment 3 2012-07-30 11:23:30 PDT

*** Bug 90181 has been marked as a duplicate of this bug. ***

Arpita Bahuguna

Comment 4 2012-08-04 04:56:07 PDT

Created attachment 156530 [details] Patch

Allan Sandfeld Jensen

Comment 5 2012-08-04 16:57:40 PDT

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.

Arpita Bahuguna

Comment 6 2012-08-06 02:28:12 PDT

(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. :)

Arpita Bahuguna

Comment 7 2012-08-06 02:42:13 PDT

Created attachment 156637 [details] Patch

Allan Sandfeld Jensen

Comment 8 2012-08-06 02:50:38 PDT

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.

Allan Sandfeld Jensen

Comment 9 2012-08-06 02:53:40 PDT

(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.

Arpita Bahuguna

Comment 10 2012-08-06 03:01:04 PDT

Created attachment 156642 [details] Patch

Arpita Bahuguna

Comment 11 2012-08-06 03:05:28 PDT

(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.

Allan Sandfeld Jensen

Comment 12 2012-08-06 03:40:05 PDT

> (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?

Allan Sandfeld Jensen

Comment 13 2012-08-06 03:49:02 PDT

(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.

Arpita Bahuguna

Comment 14 2012-08-06 04:29:03 PDT

(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.

Julien Chaffraix

Comment 15 2012-08-06 10:38:59 PDT

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.

Arpita Bahuguna

Comment 16 2012-08-07 07:17:42 PDT

Created attachment 156934 [details] Patch

Arpita Bahuguna

Comment 17 2012-08-07 07:27:40 PDT

(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.

Allan Sandfeld Jensen

Comment 18 2012-08-07 08:58:56 PDT

(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.

Julien Chaffraix

Comment 19 2012-08-07 12:18:25 PDT

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)

Arpita Bahuguna

Comment 20 2012-08-09 05:14:27 PDT

Created attachment 157438 [details] Patch

Arpita Bahuguna

Comment 21 2012-08-09 05:19:59 PDT

(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. :)

Arpita Bahuguna

Comment 22 2012-08-09 05:28:31 PDT

(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.

Julien Chaffraix

Comment 23 2012-08-09 18:12:07 PDT

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.

Arpita Bahuguna

Comment 24 2012-08-11 01:57:04 PDT

Created attachment 157860 [details] Patch

Arpita Bahuguna

Comment 25 2012-08-11 02:14:39 PDT

(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?

WebKit Review Bot

Comment 26 2012-08-11 02:33:51 PDT

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

WebKit Review Bot

Comment 27 2012-08-11 02:33:56 PDT

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

Arpita Bahuguna

Comment 28 2012-08-11 03:19:41 PDT

Created attachment 157865 [details] Patch

Allan Sandfeld Jensen

Comment 29 2012-08-11 03:31:42 PDT

(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.

Arpita Bahuguna

Comment 30 2012-08-11 04:38:55 PDT

(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.

Julien Chaffraix

Comment 31 2012-08-13 07:12:50 PDT

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.

Arpita Bahuguna

Comment 32 2012-08-13 22:23:28 PDT

Created attachment 158215 [details] Patch

Arpita Bahuguna

Comment 33 2012-08-13 22:44:04 PDT

(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.

Julien Chaffraix

Comment 34 2012-08-15 10:17:16 PDT

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).

WebKit Review Bot

Comment 35 2012-08-15 11:01:22 PDT

Comment on attachment 158215 [details] Patch Clearing flags on attachment: 158215 Committed r125684: <http://trac.webkit.org/changeset/125684>

WebKit Review Bot

Comment 36 2012-08-15 11:01:28 PDT

All reviewed patches have been landed. Closing bug.

Arpita Bahuguna

Comment 37 2012-08-15 11:07:02 PDT

(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.

Note You need to log in before you can comment on or make changes to this bug.