Bug 70396 - Focus rings are too thin in HiDPI in WebKit2
Summary: Focus rings are too thin in HiDPI in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-10-18 21:26 PDT by Beth Dakin
Modified: 2011-10-21 11:21 PDT (History)
3 users (show)

See Also:


Attachments
Patch (11.40 KB, patch)
2011-10-18 21:40 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch with tests (69.41 KB, patch)
2011-10-19 14:11 PDT, Beth Dakin
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2011-10-18 21:26:40 PDT
All focus rings are too thin in HiDPI in WebKit2.

<rdar://problem/10086876>
Comment 1 Beth Dakin 2011-10-18 21:40:55 PDT
Created attachment 111560 [details]
Patch
Comment 2 Beth Dakin 2011-10-18 21:43:19 PDT
I have a test written for this that works in WebKit2's WebKitTestRunner. It doesn't work in DRT at this time, which sets up its own HiDPI context and therefore needs the setBaseCTM code in DRT itself. I couldn't think of an obviously good way to do that since that code has to live in WebKitSystemInterface. Suggestions welcome!
Comment 3 Beth Dakin 2011-10-19 14:11:28 PDT
Created attachment 111671 [details]
Patch with tests

Here's a patch with tests!
Comment 4 mitz 2011-10-19 14:19:29 PDT
Comment on attachment 111671 [details]
Patch with tests

View in context: https://bugs.webkit.org/attachment.cgi?id=111671&action=review

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:659
> +#if USE(CG)
> +    graphicsContext->setBaseCTM(graphicsContext->getCTM());
> +#endif

I’m having trouble deciding if this is better or worse than actually getting the current base CTM, applying a deviceScaleFactor() scale to it, and setting the result as the base CTM. Or perhaps a GraphicsContext::applyDeviceScaleFactor(float) function that would do both the scaling and (on CG) the base CTM adjustment?
Comment 5 Beth Dakin 2011-10-19 15:37:26 PDT
Thanks Dan!

As I discussed with Dan on irc, I am going to continue to ponder his suggestion…I think there might be a really good design in there.

In the meantime, I committed this with revision 97886.
Comment 6 Simon Fraser (smfr) 2011-10-19 19:19:25 PDT
Comment on attachment 111671 [details]
Patch with tests

View in context: https://bugs.webkit.org/attachment.cgi?id=111671&action=review

Late to the party, sorry.

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:288
> +    AffineTransform identity;
> +    identity.makeIdentity();

I don't think makeIdentity() this is needed. AffineTransform's ctor should init it to identity, so you could just say
ctxt->setBaseCTM(AffineTransform());

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:937
> +    graphicsContext->setBaseCTM(graphicsContext->getCTM());

Do we need the same changes for compositing layers' contexts?
Comment 7 Beth Dakin 2011-10-19 21:08:17 PDT
(In reply to comment #6)
> (From update of attachment 111671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111671&action=review
> 
> Late to the party, sorry.
> 
> > Source/WebCore/platform/graphics/cg/ImageCG.cpp:288
> > +    AffineTransform identity;
> > +    identity.makeIdentity();
> 
> I don't think makeIdentity() this is needed. AffineTransform's ctor should init it to identity, so you could just say
> ctxt->setBaseCTM(AffineTransform());
> 

Oh, good point. I'll clean this up soon when I type out a slight re-design of this that's floating in my brain right now.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:937
> > +    graphicsContext->setBaseCTM(graphicsContext->getCTM());
> 
> Do we need the same changes for compositing layers' contexts?

Focus rings in compositing layers actually looked okay without any changes (at least they did when I added things with focus rings to the WebKit poster circle demo). I'm not really clear on why that is though.
Comment 8 Ilya Tikhonovsky 2011-10-20 04:39:32 PDT
Comment on attachment 111671 [details]
Patch with tests

This patch can't be built on windows.

3>GraphicsContextCG.cpp
3>..\platform\graphics\cg\GraphicsContextCG.cpp(1475) : error C3861: 'wkSetBaseCTM': identifier not found
3>Build log was saved at "file://C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\obj\WebCore\BuildLog.htm"
3>WebCore - 1 error(s), 0 warning(s)
Comment 9 Jeff Miller 2011-10-21 11:19:55 PDT
Committed r98118: <http://trac.webkit.org/changeset/98118>
Comment 10 Jeff Miller 2011-10-21 11:21:20 PDT
(In reply to comment #9)
> Committed r98118: <http://trac.webkit.org/changeset/98118>

which should fix Windows.