Bug 85348

Summary: REGRESSION: Focused TR element draws its focus outline in the wrong place
Product: WebKit Reporter: Dominic Mazzoni <dmazzoni>
Component: Layout and RenderingAssignee: Pravin D <pravind.2k4>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dglazkov, jchaffraix, pravind.2k4, robert, webkit.review.bot
Priority: P1 Keywords: EasyFix, HasReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test case: If you tab several times, you should see that the focus ring is always drawn on the first row
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-01
none
Patch
none
Archive of layout-test-results from gce-cr-linux-01
none
Patch none

Dominic Mazzoni
Reported 2012-05-01 22:56:16 PDT
You can demonstrate with this example: the second TR has focus, but the focus outline is drawn around the first row. ​<table> <tr> <td>First row</td> </tr> <tr tabIndex=0 id="row2"> <td>Second row</td> </tr> ​</table> ​<script> document.getElementById('row2').focus(); ​</script>​ You can confirm (by printing document.activeElement) that focus is on the second row, but the outline is drawn in the wrong place. This is definitely a regression;.
Attachments
Test case: If you tab several times, you should see that the focus ring is always drawn on the first row (154 bytes, text/html)
2012-07-26 15:15 PDT, Julien Chaffraix 		no flags
Details
Patch (8.23 KB, text/plain)
2012-07-27 13:02 PDT, Pravin D 		no flags
Details
Patch (7.64 KB, text/plain)
2012-07-27 14:24 PDT, Pravin D 		no flags
Details
Patch (7.64 KB, patch)
2012-07-27 14:31 PDT, Pravin D 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from gce-cr-linux-01 (1019.80 KB, application/zip)
2012-07-27 16:27 PDT, WebKit Review Bot 		no flags
Details
Patch (7.63 KB, patch)
2012-07-29 05:09 PDT, Pravin D 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from gce-cr-linux-01 (365.15 KB, application/zip)
2012-07-29 06:47 PDT, WebKit Review Bot 		no flags
Details
Patch (12.16 KB, patch)
2012-07-29 08:03 PDT, Pravin D 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2012-05-02 17:15:09 PDT
Could you please attach a full working test case? I don't get any focus ring at all for this in Safari 5.1.5.
Julien Chaffraix
Comment 2 2012-07-26 15:15:34 PDT
Created attachment 154765 [details] Test case: If you tab several times, you should see that the focus ring is always drawn on the first row We got a similar report on Chromium, attaching the test case as I think it's relevant.
Julien Chaffraix
Comment 3 2012-07-26 15:19:17 PDT
Seems like our focus ring logic doesn't add the row offset from the table: thus it is drawn on top of the first row instead of the right row. I believe the fix to be simple and is about adding the location to the focus ring's position in RenderBox::addFocusRingRects. Due to RenderBox being used for a lot of renderers, it is likely impacting other cases.
Pravin D
Comment 4 2012-07-27 05:19:30 PDT
The issue as pointed out by Julien was due to wrong paint offset for focus rings. The issue has been fixed by the changeset http://trac.webkit.org/changeset/123793 RenderBox::addFocusRingRects() is called from RenderObject::paintFocusRing() which is called from RenderObject::paintOutline(). The fix for the bug https://bugs.webkit.org/show_bug.cgi?id=92389 fixes the paint offset for row outlines which is the same offset which sent to draw focus rings... Also the issue is not repo on r123859 (Qt linux port)
Julien Chaffraix
Comment 5 2012-07-27 11:24:55 PDT
> RenderBox::addFocusRingRects() is called from RenderObject::paintFocusRing() which is called from RenderObject::paintOutline(). The fix for the bug https://bugs.webkit.org/show_bug.cgi?id=92389 fixes the paint offset for row outlines which is the same offset which sent to draw focus rings... Good analysis, this bug is related with bug 92389. However I wouldn't say they are duplicated as one is about the outline, this one is about the focus ring. It would be nice to land the test case to ensure we don't regress this case.
Pravin D
Comment 6 2012-07-27 13:02:32 PDT
Created attachment 155031 [details] Patch
Julien Chaffraix
Comment 7 2012-07-27 13:27:54 PDT
Comment on attachment 155031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155031&action=review > LayoutTests/ChangeLog:8 > + The focus outline is always drawn around the first row of the table section irrespective of the row in focus. The tense is wrong. It used to be the case but it was solved by the bug. > LayoutTests/ChangeLog:10 > + Adding regression test cases. There is one test case so it should be "Adding a regression test case." > LayoutTests/fast/table/table-row-focus-outline-paint-expected.txt:1 > +layer at (0,0) size 800x600 We don't care about the text dump so this test should be dumpAsText(true); > LayoutTests/fast/table/table-row-focus-outline-paint.html:1 > +<!DOCTYPE html> Nit: Maybe add the missing <html>. > LayoutTests/fast/table/table-row-focus-outline-paint.html:4 > +Issue: The focus outline is always drawn around the first row irrespective of the current focused row. We call this a focus ring, let's keep the naming consistent so rename all the focus outline into focus ring (including the test case name). > LayoutTests/fast/table/table-row-focus-outline-paint.html:5 > +About TestCase: The test case checks if the focus outline is drawn around the table row which currently has focus. s/About TestCase/Description/ > LayoutTests/fast/table/table-row-focus-outline-paint.html:16 > +<table > trailing space.
Pravin D
Comment 8 2012-07-27 14:24:46 PDT
Created attachment 155047 [details] Patch
Pravin D
Comment 9 2012-07-27 14:31:15 PDT
Created attachment 155050 [details] Patch
Julien Chaffraix
Comment 10 2012-07-27 14:43:44 PDT
Comment on attachment 155050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155050&action=review > LayoutTests/fast/table/table-row-focus-ring-paint.html:13 > +if(window.testRunner) > + testRunner.dumpAsText(true); You could also have pushed that to the existing <script>.
WebKit Review Bot
Comment 11 2012-07-27 16:27:25 PDT
Comment on attachment 155050 [details] Patch Attachment 155050 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13367874 New failing tests: fast/table/table-row-focus-ring-paint.html
WebKit Review Bot
Comment 12 2012-07-27 16:27:29 PDT
Created attachment 155085 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Pravin D
Comment 13 2012-07-29 05:09:18 PDT
Created attachment 155166 [details] Patch
WebKit Review Bot
Comment 14 2012-07-29 06:47:00 PDT
Comment on attachment 155166 [details] Patch Attachment 155166 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13395041 New failing tests: fast/table/table-row-focus-ring-paint.html
WebKit Review Bot
Comment 15 2012-07-29 06:47:05 PDT
Created attachment 155169 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Pravin D
Comment 16 2012-07-29 08:03:29 PDT
Created attachment 155171 [details] Patch
WebKit Review Bot
Comment 17 2012-07-30 10:42:05 PDT
Comment on attachment 155171 [details] Patch Clearing flags on attachment: 155171 Committed r124047: <http://trac.webkit.org/changeset/124047>
WebKit Review Bot
Comment 18 2012-07-30 10:42:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.