Bug 195765 - Please add API to save a PDF from the contents of a WKWebView
Summary: Please add API to save a PDF from the contents of a WKWebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: Safari 11
Hardware: Macintosh macOS 10.14
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
: 201520 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-03-14 14:38 PDT by Brad Andalman
Modified: 2019-09-10 16:29 PDT (History)
15 users (show)

See Also:


Attachments
Patch (57.50 KB, patch)
2019-09-05 15:18 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (59.86 KB, patch)
2019-09-05 16:54 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (59.86 KB, patch)
2019-09-06 12:39 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (60.31 KB, patch)
2019-09-06 13:48 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (60.16 KB, patch)
2019-09-07 14:02 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (64.05 KB, patch)
2019-09-07 15:50 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (64.39 KB, patch)
2019-09-07 16:20 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (64.38 KB, patch)
2019-09-08 11:29 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (64.54 KB, patch)
2019-09-08 11:51 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (60.07 KB, patch)
2019-09-08 15:52 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (58.21 KB, patch)
2019-09-08 16:24 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (57.69 KB, patch)
2019-09-08 16:53 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (57.87 KB, patch)
2019-09-10 12:46 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brad Andalman 2019-03-14 14:38:58 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-03-16 13:44:46 PDT
<rdar://problem/48955900>
Comment 2 Wevah 2019-06-16 23:49:15 PDT
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.)
Comment 3 Brady Eidson 2019-09-05 15:09:33 PDT
*** Bug 201520 has been marked as a duplicate of this bug. ***
Comment 4 Brady Eidson 2019-09-05 15:18:54 PDT
Created attachment 378128 [details]
Patch
Comment 5 Tim Horton 2019-09-05 15:45:54 PDT
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
Comment 6 Said Abou-Hallawa 2019-09-05 16:12:04 PDT
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);
Comment 7 Brady Eidson 2019-09-05 16:54:43 PDT
Created attachment 378139 [details]
Patch
Comment 8 Brady Eidson 2019-09-05 16:55:43 PDT
(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.
Comment 9 Brady Eidson 2019-09-06 12:39:37 PDT
Created attachment 378218 [details]
Patch
Comment 10 Brady Eidson 2019-09-06 13:48:31 PDT
Created attachment 378228 [details]
Patch
Comment 11 Brady Eidson 2019-09-07 14:02:19 PDT
Created attachment 378300 [details]
Patch
Comment 12 Brady Eidson 2019-09-07 15:50:53 PDT
Created attachment 378301 [details]
Patch
Comment 13 Brady Eidson 2019-09-07 16:20:37 PDT
Created attachment 378303 [details]
Patch
Comment 14 Brady Eidson 2019-09-08 11:29:32 PDT
Created attachment 378330 [details]
Patch
Comment 15 Brady Eidson 2019-09-08 11:51:57 PDT
Created attachment 378331 [details]
Patch
Comment 16 Brady Eidson 2019-09-08 13:09:42 PDT
The <win> bot build failure has nothing to do with this patch.
Comment 17 Tim Horton 2019-09-08 14:35:25 PDT
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?
Comment 18 Brady Eidson 2019-09-08 14:51:28 PDT
(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.
Comment 19 Brady Eidson 2019-09-08 15:52:26 PDT
Created attachment 378336 [details]
Patch
Comment 20 Brady Eidson 2019-09-08 16:24:49 PDT
Created attachment 378337 [details]
Patch
Comment 21 Brady Eidson 2019-09-08 16:53:13 PDT
Created attachment 378339 [details]
Patch
Comment 22 Brady Eidson 2019-09-10 12:46:03 PDT
Created attachment 378476 [details]
Patch
Comment 23 Simon Fraser (smfr) 2019-09-10 13:00:39 PDT
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?
Comment 24 WebKit Commit Bot 2019-09-10 16:12:56 PDT
Comment on attachment 378476 [details]
Patch

Clearing flags on attachment: 378476

Committed r249739: <https://trac.webkit.org/changeset/249739>
Comment 25 WebKit Commit Bot 2019-09-10 16:12:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Brady Eidson 2019-09-10 16:29:35 PDT
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!