85348 – REGRESSION: Focused TR element draws its focus outline in the wrong place

RESOLVED FIXED 85348

REGRESSION: Focused TR element draws its focus outline in the wrong place

https://bugs.webkit.org/show_bug.cgi?id=85348

Summary REGRESSION: Focused TR element draws its focus outline in the wrong place

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	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P1

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component Layout and Rendering

Assignee

Pravin D

Reported

2012-05-01 22:56 PDT

Modified

2012-07-30 10:42 PDT History

CC List

6 users Show

URL

Keywords EasyFix, HasReduction

Depends on

Blocks