Summary: | Focus rings are too thin in HiDPI in WebKit2 | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||
Component: | Layout and Rendering | Assignee: | Beth Dakin <bdakin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bdakin, jeffm, simon.fraser | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Beth Dakin
2011-10-18 21:26:40 PDT
Created attachment 111560 [details]
Patch
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! 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? 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 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? (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 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)
Committed r98118: <http://trac.webkit.org/changeset/98118> (In reply to comment #9) > Committed r98118: <http://trac.webkit.org/changeset/98118> which should fix Windows. |