Bug 195765 – Please add API to save a PDF from the contents of a WKWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Brad Andalman]

I have an application which – as a critical feature – creates a PDF from the contents of a webview. This PDF isn't merely a container for an image, but is a "real" PDF that contains embedded fonts, etc. The way this is currently accomplished is by rendering a view which contains a legacy WebView (there are actually three, but that is largely beside-the-point) into a PDFContext. It is important that these PDFs match what is in the view *exactly*, which is why we cannot use WebView's PrintOperation, for example.

We understand that WebViews will be going away at some point in the future, and wanted to adopt WKWebView. Unfortunately, the above design results in a blank PDF when WKWebViews are used. Ideally, this would "just work" and switching to WKWebViews would be trivial for us.

However, it seems like that might violate the design of WKWebViews. If that's the case, please add API to WKWebView to save a PDF from its contents. Perhaps something similar to takeSnapshot would work? Or perhaps takeSnapshot would work if WKSnapshotConfiguration had a "format" property? 

I've filed this as normal severity, but for my application this is clearly a blocker: we cannot adopt WKWebViews, and must continue using the legacy WebViews, until this is addressed.

[Radar WebKit Bug Importer]

<rdar://problem/48955900>

[Wevah]

Seconding this; I stumbled upon being able to capture a WebView as PDF (with clickable links) years ago via -dataWithPDFInsideRect:, and having to remove this feature would be a bummer. (Tangentially, compiling an app with the 10.14 SDK seems to make -dataWithPDFInsideRect: on WebView produce PDFs that can't be opened properly with non-Apple apps.)

[Brady Eidson]

*** Bug 201520 has been marked as a duplicate of this bug. ***

[Brady Eidson]

Created attachment 378128 [details]
Patch

[Tim Horton]

Comment on attachment 378128 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5032
> +    // FIXME: <rdar://problem/55081578> PDF exporting on iOS should respect WKSnapshotConfiguration.afterScreenUpdates

I don't think there's anything for it to do? That's there because the image API can snapshot without bouncing to the Web Content process (and thus won't naturally include changes that haven't arrived in the latest transaction), but in the PDF case you always get everything because you have to go through the Web Content process.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:435
> +- (void)_takePDFSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void (^)(NSData * pdfSnapshotData, NSError * error))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

No spaces after the stars

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2412
> +        auto dictionary = adoptCF(CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +        CGRect mediaBox = CGRectMake(0, 0, bitmapSize.width(), bitmapSize.height());
> +        auto mediaBoxData = adoptCF(CFDataCreate(NULL, (const UInt8 *)&mediaBox, sizeof(CGRect)));
> +        CFDictionarySetValue(dictionary.get(), kCGPDFContextMediaBox, mediaBoxData.get());

ObjC dictionary literal syntax is much nicer

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4786
> +    auto originalLayoutViewportOverrideRect = frameView.layoutViewportOverrideRect();
> +    frameView.setLayoutViewportOverrideRect(LayoutRect(snapshotRect));

Ooh you should add a test for this code (by having a fixed position thing and testing that it ends up in the right place). I didn't test because I'm not a good person like you, but now you can!

> Tools/MiniBrowser/MiniBrowser.entitlements:4
>  <dict>

Unfortunate that the sorting and read-only -> read-write change are together

> Tools/TestWebKitAPI/Configurations/TestWebKitAPILibrary.xcconfig:32
> +

weird extra space here

[Said Abou-Hallawa]

Comment on attachment 378128 [details]
Patch

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

> Tools/TestWebKitAPI/cocoa/TestPDFDocument.mm:121
> +    auto context = adoptCF(CGBitmapContextCreate(NULL, boundsRect.size.width, boundsRect.size.height, 8, 0, colorSpace.get(), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big));

You do not have to draw the whole PDF to know the color at a point. You can create 1x1 bitmap context, transform the context to this point and then draw the PDF to the context.

unsigned char pixel[4];
auto context = adoptCF(CGBitmapContextCreate(pixel, 1, 1, 8, sizeof(pixel), colorSpace.get(), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big));
context.translate(-x, -y);

[Brady Eidson]

Created attachment 378139 [details]
Patch

[Brady Eidson]

(In reply to Said Abou-Hallawa from comment #6)
> Comment on attachment 378128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378128&action=review
> 
> > Tools/TestWebKitAPI/cocoa/TestPDFDocument.mm:121
> > +    auto context = adoptCF(CGBitmapContextCreate(NULL, boundsRect.size.width, boundsRect.size.height, 8, 0, colorSpace.get(), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big));
> 
> You do not have to draw the whole PDF to know the color at a point. You can
> create 1x1 bitmap context, transform the context to this point and then draw
> the PDF to the context.
> 
> unsigned char pixel[4];
> auto context = adoptCF(CGBitmapContextCreate(pixel, 1, 1, 8, sizeof(pixel),
> colorSpace.get(), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big));
> context.translate(-x, -y);

Oh super neat! Will try out this change the next upload.

[Brady Eidson]

Created attachment 378218 [details]
Patch

[Brady Eidson]

Created attachment 378228 [details]
Patch

[Brady Eidson]

Created attachment 378300 [details]
Patch

[Brady Eidson]

Created attachment 378301 [details]
Patch

[Brady Eidson]

Created attachment 378303 [details]
Patch

[Brady Eidson]

Created attachment 378330 [details]
Patch

[Brady Eidson]

Created attachment 378331 [details]
Patch

[Brady Eidson]

The <win> bot build failure has nothing to do with this patch.

[Tim Horton]

Comment on attachment 378331 [details]
Patch

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

Plus the one offline comment

> Source/WebCore/rendering/PaintPhase.h:70
> +    AnnotateLinks               = 1 << 12,

This one is weird enough it probably deserves a comment

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:-4216
> -		537CF84822EFD72000C6EBB3 /* Check .xcfilelists */ = {

Where did this go??

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5048
> +					"$(SYSTEM_LIBRARY_DIR)/Frameworks/Quartz.framework/Versions/A/Frameworks",

Shouldn't this be in an xcconfig?

[Brady Eidson]

(In reply to Tim Horton from comment #17)
> Comment on attachment 378331 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378331&action=review
> 
> Plus the one offline comment
> 
> > Source/WebCore/rendering/PaintPhase.h:70
> > +    AnnotateLinks               = 1 << 12,
> 
> This one is weird enough it probably deserves a comment
> 
> > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:-4216
> > -		537CF84822EFD72000C6EBB3 /* Check .xcfilelists */ = {
> 
> Where did this go??

No clue - I did not consciously remove it.

I will restore it manually, though.

> > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5048
> > +					"$(SYSTEM_LIBRARY_DIR)/Frameworks/Quartz.framework/Versions/A/Frameworks",
> 
> Shouldn't this be in an xcconfig?

Per the patch, it actually *is* in an xcconfig...  I'm not actually sure how it got into the xcodeproj.

I'll cleanup the xcodeproj stuff before landing.

[Brady Eidson]

Created attachment 378336 [details]
Patch

[Brady Eidson]

Created attachment 378337 [details]
Patch

[Brady Eidson]

Created attachment 378339 [details]
Patch

[Brady Eidson]

Created attachment 378476 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 378476 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:2036
> +    return element() && element()->isLink() && (document().printing() || (view().frameView().paintBehavior() & PaintBehavior::AnnotateLinks));

Would be nicer to set the PaintBehavior::AnnotateLinks bit when printing.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5040
> +- (void)_takePDFSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void (^)(NSData *, NSError *))completionHandler

Should the format (PDF) be part of the WKSnapshotConfiguration?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5057
> +            handler(nil, createNSError(WKErrorUnknown).get());

Can we do better than "unknown error"?

> Source/WebKit/UIProcess/WebPageProxy.cpp:7754
> +#if PLATFORM(COCOA)
> +void WebPageProxy::drawToPDF(FrameIdentifier frameID, const Optional<FloatRect>& rect, DrawToPDFCallback::CallbackFunction&& callback)

Why not put it in WebPageProxyCocoa.mm?

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:263
> +        auto mediaBoxData = adoptCF(CFDataCreate(NULL, (const UInt8 *)&mediaBox, sizeof(CGRect)));

That's pretty crazy but looks correct.

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:276
> +        CGPDFContextBeginPage(pdfContext.get(), dictionary);
> +
> +        GraphicsContext graphicsContext { pdfContext.get() };
> +        graphicsContext.scale({ 1, -1 });
> +        graphicsContext.translate(0, -bitmapSize.height());
> +
> +        paintSnapshotAtSize(rect, bitmapSize, options, *coreFrame, *frameView, graphicsContext);
> +
> +        CGPDFContextEndPage(pdfContext.get());

Use some {} to scope the GraphicsContext between the begin page/end page

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4719
>  #if PLATFORM(COCOA)
> +void WebPage::drawToPDF(FrameIdentifier frameID, const Optional<FloatRect>& rect, CallbackID callbackID)

WebPageCocoa.mm?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4735
> +    frameView.setLayoutViewportOverrideRect(LayoutRect(snapshotRect));

Do you really want position:fixed thing laid out relative to the snapshotRect if it's just a small portion of the document? Probably not?

[WebKit Commit Bot]

Comment on attachment 378476 [details]
Patch

Clearing flags on attachment: 378476

Committed r249739: <https://trac.webkit.org/changeset/249739>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

Sorry for missing these comments before CQ landed.

(In reply to Simon Fraser (smfr) from comment #23)
> Comment on attachment 378476 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378476&action=review
> 
> > Source/WebCore/rendering/RenderElement.cpp:2036
> > +    return element() && element()->isLink() && (document().printing() || (view().frameView().paintBehavior() & PaintBehavior::AnnotateLinks));
> 
> Would be nicer to set the PaintBehavior::AnnotateLinks bit when printing.

I agree the printing codepath has room for cleanup, this included! But I also suspect our printing tests don't cover annotations in the generated PDF, and therefore the risk of regression is... non-trivial.
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5040
> > +- (void)_takePDFSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void (^)(NSData *, NSError *))completionHandler
> 
> Should the format (PDF) be part of the WKSnapshotConfiguration?

The current API in its current form literally cannot handle this new use-case, so we had to add PDF-specific API.

The hope is that we can get support from other teams to make the current API work, at which point we would fold the "make PDF" bit onto snapshot configuration.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5057
> > +            handler(nil, createNSError(WKErrorUnknown).get());
> 
> Can we do better than "unknown error"?

Not with API, sadly. (We need a big WKError API revamp, not just related to this patch!

> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:4735
> > +    frameView.setLayoutViewportOverrideRect(LayoutRect(snapshotRect));
> 
> Do you really want position:fixed thing laid out relative to the
> snapshotRect if it's just a small portion of the document? Probably not?

Discussed this IRL - Great point, and I'm working on it now!