WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
177582
Request for PDF URL targets for specific pages, sections, etc.
https://bugs.webkit.org/show_bug.cgi?id=177582
Summary
Request for PDF URL targets for specific pages, sections, etc.
Aishwarya Nirmal
Reported
2017-09-27 16:50:31 PDT
<
rdar://33496301
>
Attachments
Patch
(2.37 KB, patch)
2017-09-27 16:57 PDT
,
Aishwarya Nirmal
no flags
Details
Formatted Diff
Diff
Patch
(2.48 KB, patch)
2017-09-28 10:32 PDT
,
Aishwarya Nirmal
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Aishwarya Nirmal
Comment 1
2017-09-27 16:57:56 PDT
Created
attachment 322043
[details]
Patch
Tim Horton
Comment 2
2017-09-27 17:05:04 PDT
Comment on
attachment 322043
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=322043&action=review
> Source/WebKit/ChangeLog:5 > + <
rdar://problem/33496301
>
This should be 5692679; the one you mentioned is a dupe.
> Source/WebKit/WebProcess/Plugins/PDF/PDFLayerControllerSPI.h:71 > +@property (nonatomic, strong) NSString* URLFragment;
The asterisk is on the wrong side; we put them on the left for C++ types and on the right for ObjC types (it's weird, I know).
> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1039 > + m_pdfLayerController.get().URLFragment = pdfURL.substring(index);
This should get wrapped in an __MAC_OS_X_VERSION_MIN_REQUIRED ifdef, because it's not available in shipping versions of macOS.
Tim Horton
Comment 3
2017-09-27 17:07:30 PDT
Comment on
attachment 322043
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=322043&action=review
> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1037 > + size_t index = pdfURL.reverseFind('#');
I bet our URL parser has a better way to do this! We should ask Alex.
>> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1039 >> + m_pdfLayerController.get().URLFragment = pdfURL.substring(index); > > This should get wrapped in an __MAC_OS_X_VERSION_MIN_REQUIRED ifdef, because it's not available in shipping versions of macOS.
Also I generally prefer the [m_pdfLayerController setURLFragment:] variant instead of dot notation in the case where you've got a RetainPtr. (.get() is so ugly)
Tim Horton
Comment 4
2017-09-27 17:08:22 PDT
Comment on
attachment 322043
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=322043&action=review
>> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1037 >> + size_t index = pdfURL.reverseFind('#'); > > I bet our URL parser has a better way to do this! We should ask Alex.
Actually, I don't even think we need to ask Alex; URL has fragmentIdentifier()
Wenson Hsieh
Comment 5
2017-09-28 10:05:28 PDT
(In reply to Tim Horton from
comment #2
)
> Comment on
attachment 322043
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=322043&action=review
> > > Source/WebKit/ChangeLog:5 > > + <
rdar://problem/33496301
> > > This should be 5692679; the one you mentioned is a dupe. > > > Source/WebKit/WebProcess/Plugins/PDF/PDFLayerControllerSPI.h:71 > > +@property (nonatomic, strong) NSString* URLFragment; > > The asterisk is on the wrong side; we put them on the left for C++ types and > on the right for ObjC types (it's weird, I know). > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1039 > > + m_pdfLayerController.get().URLFragment = pdfURL.substring(index); > > This should get wrapped in an __MAC_OS_X_VERSION_MIN_REQUIRED ifdef, because > it's not available in shipping versions of macOS.
If I understand correctly, this should be a MIN_REQUIRED check in the implementation file and a MAX_ALLOWED check in PDFLayerControllerSPI.h?
Aishwarya Nirmal
Comment 6
2017-09-28 10:32:06 PDT
Created
attachment 322096
[details]
Patch
Aishwarya Nirmal
Comment 7
2017-09-28 10:34:43 PDT
(In reply to Aishwarya Nirmal from
comment #0
)
> <
rdar://33496301
>
The original bug is <
rdar://5692679
>
Tim Horton
Comment 8
2017-09-28 11:13:32 PDT
The title from 5692679 would have been preferable (it makes more sense), for future reference.
WebKit Commit Bot
Comment 9
2017-09-28 11:40:45 PDT
Comment on
attachment 322096
[details]
Patch Clearing flags on attachment: 322096 Committed
r222620
: <
http://trac.webkit.org/changeset/222620
>
WebKit Commit Bot
Comment 10
2017-09-28 11:40:46 PDT
All reviewed patches have been landed. Closing bug.
Tim Horton
Comment 11
2017-09-28 12:35:37 PDT
Minor follow up in
https://trac.webkit.org/changeset/222624/webkit
Karim Ardalan
Comment 12
2022-05-12 08:46:44 PDT
This feature is working in Safari for MacOS but still isn't working for Safari on iOS/iPadOS. Could you please fix this?
https://helpx.adobe.com/acrobat/kb/link-html-pdf-page-acrobat.html
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug