| Summary: | [Qt] Focus rings are ugly, rects should be united instead of drawn individually | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||||||
| Component: | Layout and Rendering | Assignee: | Renata Hodovan <rhodovan.u-szeged> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | abarth, commit-queue, eric, ossy, rhodovan.u-szeged, tonikitoo, webkit.review.bot, yael | ||||||||||||
| Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Andreas Kling
2010-11-22 19:14:41 PST
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. |