Bug 70396 – Focus rings are too thin in HiDPI in WebKit2

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

[Beth Dakin]

All focus rings are too thin in HiDPI in WebKit2.

<rdar://problem/10086876>

[Beth Dakin]

Created attachment 111560 [details]
Patch

[Beth Dakin]

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!

[Beth Dakin]

Created attachment 111671 [details]
Patch with tests

Here's a patch with tests!

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?

[Beth Dakin]

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.

[Simon Fraser (smfr)]

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?

[Beth Dakin]

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

[Ilya Tikhonovsky]

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)

[Jeff Miller]

Committed r98118: <http://trac.webkit.org/changeset/98118>

[Jeff Miller]

(In reply to comment #9)
> Committed r98118: <http://trac.webkit.org/changeset/98118>

which should fix Windows.