WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
74864
There is additional space not belonged to a table between the table cells
https://bugs.webkit.org/show_bug.cgi?id=74864
Summary
There is additional space not belonged to a table between the table cells
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.
Top of Page
Format For Printing
XML
Clone This Bug