Bug 49953

Created attachment 74627 [details] QtWebKit screenshot - focused link split over multiple lines

Created attachment 74628 [details] Chromium screenshot - focused link split over multiple lines

(In reply to comment #0) > We currently draw each rectangle individually in GraphicsContext::drawFocusRing(Vector<IntRect>, ...) > > We should join the rects and paint them as a bounding path, like other ports do. > > Screenshots from QtWebKit and Chromium coming. Thank you Andreas! Antonio Gomes & I were talking about exactly the same thing. The focus ring in Qt is ugly and hard to see in many cases.

Created attachment 76392 [details] QtWebKit screenshot - r73897 Since the addition of support for color and width for focus rings in <http://trac.webkit.org/changeset/73361>, QtWebKit trunk looks slightly ridiculous (see screenshot.)

(In reply to comment #4) > Created an attachment (id=76392) [details] > QtWebKit screenshot - r73897 > > Since the addition of support for color and width for focus rings in <http://trac.webkit.org/changeset/73361>, QtWebKit trunk looks slightly ridiculous (see screenshot.) Agreed. I already offered to take over this bug :-)

(In reply to comment #5) > (In reply to comment #4) > > Created an attachment (id=76392) [details] [details] > > QtWebKit screenshot - r73897 > > > > Since the addition of support for color and width for focus rings in <http://trac.webkit.org/changeset/73361>, QtWebKit trunk looks slightly ridiculous (see screenshot.) > > Agreed. I already offered to take over this bug :-) Go for it, I doubt Reni was going to work on this anytime soon. :-)

Created attachment 76487 [details] Patch. Create a QPainterPath and add to it the focus rects, one at a time. Combine the 2 drawFocusRing functions into one function drawFocusRingForPath. Kling, the size of this patch is due to the images from the tests, code changes are very small :-)

Comment on attachment 76487 [details] Patch. Much better! r=me

Comment on attachment 76487 [details] Patch. go, go, go :)

Comment on attachment 76487 [details] Patch. Rejecting attachment 76487 [details] from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: .... fast/clip .................... fast/compact ... fast/constructors . fast/cookies . fast/css-generated-content ....................................... fast/css .......................................................................................................... fast/css/focus-ring-detached.html -> failed Exiting early after 1 failures. 6428 tests run. 111.53s total testing time 6427 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 4 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/6961111

Created attachment 76554 [details] Patch It was suggested on IRC that I will not provide mac results. Updated patch is teh same as before, minus mac results.

Comment on attachment 76554 [details] Patch Rejecting attachment 76554 [details] from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: s/websocket/tests/workers ...... http/tests/workers ..... http/tests/xhtmlmp . http/tests/xmlhttprequest ............................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ......... 547.39s total testing time 22190 test cases (99%) succeeded 2 test cases (<1%) were new 13 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/6934134

Committed 74069. <http://trac.webkit.org/changeset/74069>

http://trac.webkit.org/changeset/74069 might have broken SnowLeopard Intel Release (Tests) The following tests are not passing: fast/css/focus-ring-detached.html fast/css/focus-ring-multiline.html

It is much better now, but still looks bad. The focus ring could be more thin. Yael, are you on it or plan to?

(In reply to comment #15) > It is much better now, but still looks bad. The focus ring could be more thin. Yael, are you on it or plan to? I have been trying to make it look better, but I don't have a solution yet. Ideally, we should use a gradient to draw the focus ring, like the mac port does. See an example in <http://imagepaste.nullnetwork.net/viewimage.php?id=1591>. When I tried to use Qt's radial gradient, I got the result in <http://imagepaste.nullnetwork.net/viewimage.php?id=1590>, which is not what I wanted. I'll be happy to get some help fixing it.

(In reply to comment #16) > (In reply to comment #15) > > It is much better now, but still looks bad. The focus ring could be more thin. Yael, are you on it or plan to? > > I have been trying to make it look better, but I don't have a solution yet. > > Ideally, we should use a gradient to draw the focus ring, like the mac port does. See an example in <http://imagepaste.nullnetwork.net/viewimage.php?id=1591>. > When I tried to use Qt's radial gradient, I got the result in <http://imagepaste.nullnetwork.net/viewimage.php?id=1590>, which is not what I wanted. I'll be happy to get some help fixing it. Ok. Is there a follow up filed? Should we reopen?

(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > It is much better now, but still looks bad. The focus ring could be more thin. Yael, are you on it or plan to? > > > > I have been trying to make it look better, but I don't have a solution yet. > > > > Ideally, we should use a gradient to draw the focus ring, like the mac port does. See an example in <http://imagepaste.nullnetwork.net/viewimage.php?id=1591>. > > When I tried to use Qt's radial gradient, I got the result in <http://imagepaste.nullnetwork.net/viewimage.php?id=1590>, which is not what I wanted. I'll be happy to get some help fixing it. > > Ok. Is there a follow up filed? Should we reopen? I think we need a new bug. This bug was filed before I added support for outline-width, which caused this problem.

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