Bug 49953 – [Qt] Focus rings are ugly, rects should be united instead of drawn individually

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Andreas Kling]

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.

[Andreas Kling]

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

[Andreas Kling]

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

[Yael]

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

[Andreas Kling]

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

[Yael]

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

[Andreas Kling]

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

[Yael]

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

[Andreas Kling]

Comment on attachment 76487 [details]
Patch.

Much better! r=me

[Csaba Osztrogonác]

Comment on attachment 76487 [details]
Patch.

go, go, go :)

[WebKit Commit Bot]

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

[Yael]

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.

[WebKit Commit Bot]

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

[Yael]

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

[WebKit Review Bot]

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

[Antonio Gomes]

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

[Yael]

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

[Antonio Gomes]

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

[Yael]

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

[Yael]

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