Add scrolling options to PDFJS context menus. #2417

sammygill · 2022-07-14T17:44:34Z

`d50f30c`

Add scrolling options to PDFJS context menus.
https://bugs.webkit.org/show_bug.cgi?id=237059
rdar://89315125

Reviewed by Aditya Keerthi.

This patch adds scrolling options to the context menu when right
clicking on a PDF that is being rendered in PDFJS. It is important to
note that this patch only adds the options to the context menu in
appearance only. Linking the options to the actual implementation in
PDFJS will be done in another patch.

By taking advantage of the current context menu code, such as in
ContextMenuController, we can try to keep the PDF code as close to the
rest of the context menu code. For example, we would have have to go
through WebPageProxy::showPDFContextMenu inside WebPageProxyMac, which
the current PDFPlugin code does.

The only special consideration that needs to be made is within
ContextMenuController::populate. We need to be able to determine that
the PDF context menu items must be added to the context menu. We can
do this by determining that the current frame is an iframe
(!frame->isMainFrame) and that the owning document is a PDFDocument
(frame->ownerElement()->document().isPDFDocument()). Similarly, in
already existing cases where the code checks for frame->page(), we must
add an extra condition that the owning document is NOT a PDFDocument so
that we do not add any extra context menu items to PDF context menus and
remain consistent with the current PDF context menu.

* Source/WebCore/en.lproj/Localizable.strings:
* Source/WebCore/page/ContextMenuController.cpp:
(WebCore::ContextMenuController::checkOrEnableIfNeeded const):
(WebCore::ContextMenuController::populate):
Goes through and determines where to add the PDF context menu items. We
need to make sure that the normal context menu remain the same (when
not viewing a PDF) and also to make sure that the PDF context menu
remains consistent with the current implementation.

* Source/WebCore/platform/ContextMenuItem.cpp:
(WebCore::isValidContextMenuAction):
* Source/WebCore/platform/ContextMenuItem.h:
Adds the new context menu scrolling items to the ContextMenuAction
enum.

* Source/WebCore/platform/LocalizedStrings.cpp:
(WebCore::contextMenuItemPDFSinglePage):
(WebCore::contextMenuItemPDFSinglePageContinuous):
(WebCore::contextMenuItemPDFTwoPages):
(WebCore::contextMenuItemPDFTwoPagesContinuous):
* Source/WebCore/platform/LocalizedStrings.h:
* Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:
(toAction):
(toTag):

Canonical link: https://commits.webkit.org/253130@main

nt1m

If this is specific to PDF.js, it should probably not touch PDFPlugin code

sammygill · 2022-07-14T22:33:01Z

If this is specific to PDF.js, it should probably not touch PDFPlugin code

It looks like to get outside of PDFPlugin we will want to register some event handler outside of it. WebCore::callDefaultEventHandlersInBubblingOrder eventually calls HTMLPluginElement::defaultEventHandler(). Maybe this could be done inside WebCore::PDFDocument or is there perhaps a better area?

sammygill · 2022-07-14T23:02:51Z

If this is specific to PDF.js, it should probably not touch PDFPlugin code

It looks like to get outside of PDFPlugin we will want to register some event handler outside of it. WebCore::callDefaultEventHandlersInBubblingOrder eventually calls HTMLPluginElement::defaultEventHandler(). Maybe this could be done inside WebCore::PDFDocument or is there perhaps a better area?

Actually I don't think PDFDocument is the right place to do this. I'll need to look around a bit

cdumez · 2022-07-21T18:00:29Z

This PR title seems overly generic. It should probably say something about PDFjs or PDF.

sammygill · 2022-07-21T18:12:23Z

This PR title seems overly generic. It should probably say something about PDFjs or PDF.

Updated!

cdumez · 2022-07-21T18:39:52Z

Source/WebCore/page/ContextMenuController.cpp

-                    appendItem(StopItem, m_contextMenu.get());
-                else
-                    appendItem(ReloadItem, m_contextMenu.get());
+                if (frame->isMainFrame() || (frame->page() && !frame->ownerElement()->document().isPDFDocument())) {


Did you mean to null check frame->ownerElement() instead of frame->page()?

Added the null pointer check here. Also removed the frame->page() check since the more specific if branches are checking for them along with other checks below.

cdumez · 2022-07-21T18:41:20Z

Source/WebCore/page/ContextMenuController.cpp

 #endif
                }

-                if (frame->page() && !frame->isMainFrame())
+                if (frame->page() && !frame->isMainFrame() && !frame->ownerElement()->document().isPDFDocument()) 


What guarantees that frame->ownerElement() is non null?

I think I had some misunderstandings between frames and pages. I will definitely need to add a null check for that. If we got rid of the frame->page() check here would that be ok? Since a frame could get detached from a page (although I don't entirely know what that scenario would be), would we want to check that or do you not think it would matter in this case? Also, it looks like I might want to add a null check for the document as well?

might want to add a null check for the document as well?
No, document() is a reference, not a pointer so it cannot be null.

But when you dereference this pointer (frame->ownerElement()) without a null check, this looks dangerous.

Added the frame->ownerElement null pointer check here

Source/WebCore/platform/ContextMenuItem.h

nt1m · 2022-07-22T01:26:11Z

Can you guard the new code behind ENABLE(PDFJS) ?

Source/WebCore/page/ContextMenuController.cpp

Source/WebCore/platform/ContextMenuItem.cpp

Source/WebCore/page/ContextMenuController.cpp

pxlcoder · 2022-08-04T00:48:37Z

Source/WebCore/platform/ContextMenuItem.cpp

+    case ContextMenuAction::ContextMenuItemPDFSinglePageContinuous:
+    case ContextMenuAction::ContextMenuItemPDFTwoPages:
+    case ContextMenuAction::ContextMenuItemPDFTwoPagesContinuous:
+        return true;


@sgill26 It would be good to move these up and share the return true; with the other cases in a follow-up patch (maybe the patch where you hook up the context menu items into PDF.js).

webkit-commit-queue · 2022-08-04T00:56:06Z

Commit message contains (OOPS!), blocking PR #2417

webkit-commit-queue · 2022-08-04T11:35:38Z

Commit message contains (OOPS!) and no reviewer found, blocking PR #2417

pxlcoder · 2022-08-04T17:15:15Z

Source/WebCore/page/ContextMenuController.cpp

+ContextMenuItem SinglePageItem(ActionType, ContextMenuItemPDFSinglePage, contextMenuItemPDFSinglePage());
+ContextMenuItem SinglePageContinuousItem(ActionType, ContextMenuItemPDFSinglePageContinuous, contextMenuItemPDFSinglePageContinuous());
+ContextMenuItem TwoPagesItem(ActionType, ContextMenuItemPDFTwoPages, contextMenuItemPDFTwoPages());
+ContextMenuItem TwoPagesContinuousItem(ActionType, ContextMenuItemPDFTwoPagesContinuous, contextMenuItemPDFTwoPagesContinuous());


Nit: Indentation.

https://bugs.webkit.org/show_bug.cgi?id=237059 rdar://89315125 Reviewed by Aditya Keerthi. This patch adds scrolling options to the context menu when right clicking on a PDF that is being rendered in PDFJS. It is important to note that this patch only adds the options to the context menu in appearance only. Linking the options to the actual implementation in PDFJS will be done in another patch. By taking advantage of the current context menu code, such as in ContextMenuController, we can try to keep the PDF code as close to the rest of the context menu code. For example, we would have have to go through WebPageProxy::showPDFContextMenu inside WebPageProxyMac, which the current PDFPlugin code does. The only special consideration that needs to be made is within ContextMenuController::populate. We need to be able to determine that the PDF context menu items must be added to the context menu. We can do this by determining that the current frame is an iframe (!frame->isMainFrame) and that the owning document is a PDFDocument (frame->ownerElement()->document().isPDFDocument()). Similarly, in already existing cases where the code checks for frame->page(), we must add an extra condition that the owning document is NOT a PDFDocument so that we do not add any extra context menu items to PDF context menus and remain consistent with the current PDF context menu. * Source/WebCore/en.lproj/Localizable.strings: * Source/WebCore/page/ContextMenuController.cpp: (WebCore::ContextMenuController::checkOrEnableIfNeeded const): (WebCore::ContextMenuController::populate): Goes through and determines where to add the PDF context menu items. We need to make sure that the normal context menu remain the same (when not viewing a PDF) and also to make sure that the PDF context menu remains consistent with the current implementation. * Source/WebCore/platform/ContextMenuItem.cpp: (WebCore::isValidContextMenuAction): * Source/WebCore/platform/ContextMenuItem.h: Adds the new context menu scrolling items to the ContextMenuAction enum. * Source/WebCore/platform/LocalizedStrings.cpp: (WebCore::contextMenuItemPDFSinglePage): (WebCore::contextMenuItemPDFSinglePageContinuous): (WebCore::contextMenuItemPDFTwoPages): (WebCore::contextMenuItemPDFTwoPagesContinuous): * Source/WebCore/platform/LocalizedStrings.h: * Source/WebKitLegacy/mac/WebView/WebHTMLView.mm: (toAction): (toTag): Canonical link: https://commits.webkit.org/253130@main

webkit-commit-queue · 2022-08-04T20:47:47Z

Committed 253130@main (d50f30c): https://commits.webkit.org/253130@main

Reviewed commits have been landed. Closing PR #2417 and removing active labels.

sammygill self-assigned this Jul 14, 2022

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from b55bd47 to 1ff6586 Compare July 14, 2022 17:56

sammygill changed the title ~~Need a short description (OOPS!).~~ Create context menu with scrolling options. Jul 14, 2022

sammygill added PDF For bugs in WebKit's built-in PDF support. WebKit Nightly Build labels Jul 14, 2022

sammygill marked this pull request as draft July 14, 2022 17:57

nt1m requested changes Jul 14, 2022

View reviewed changes

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from 1ff6586 to c72b4aa Compare July 15, 2022 23:15

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jul 15, 2022

sammygill removed the merging-blocked Applied to prevent a change from being merged label Jul 19, 2022

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from c72b4aa to d670863 Compare July 19, 2022 00:16

sammygill changed the title ~~Create context menu with scrolling options.~~ Need a short description (OOPS!). Jul 19, 2022

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jul 19, 2022

sammygill removed the merging-blocked Applied to prevent a change from being merged label Jul 21, 2022

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from d670863 to c005256 Compare July 21, 2022 17:56

sammygill changed the title ~~Need a short description (OOPS!).~~ Create context menu with scrolling options. Jul 21, 2022

sammygill marked this pull request as ready for review July 21, 2022 17:57

sammygill requested a review from cdumez as a code owner July 21, 2022 17:57

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from c005256 to 5b20186 Compare July 21, 2022 18:11

sammygill changed the title ~~Create context menu with scrolling options.~~ Add scrolling options to PDFJS context menus. Jul 21, 2022

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from 5b20186 to dc8d657 Compare July 21, 2022 18:40

cdumez reviewed Jul 21, 2022

View reviewed changes

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from dc8d657 to f86f94d Compare July 21, 2022 21:37

pxlcoder reviewed Jul 22, 2022

View reviewed changes

Source/WebCore/platform/ContextMenuItem.h Outdated Show resolved Hide resolved

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from f86f94d to 3e8dad2 Compare July 22, 2022 21:09

pxlcoder approved these changes Aug 2, 2022

View reviewed changes

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from 2732914 to 3deaa11 Compare August 2, 2022 22:07

pxlcoder reviewed Aug 2, 2022

View reviewed changes

Source/WebCore/page/ContextMenuController.cpp Outdated Show resolved Hide resolved

Source/WebCore/page/ContextMenuController.cpp Outdated Show resolved Hide resolved

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch 2 times, most recently from 6adc716 to 704f9bd Compare August 3, 2022 16:57

pxlcoder added the merge-queue Applied to send a pull request to merge-queue label Aug 4, 2022

pxlcoder reviewed Aug 4, 2022

View reviewed changes

webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Aug 4, 2022

aj062 added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Aug 4, 2022

webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Aug 4, 2022

aj062 mentioned this pull request Aug 4, 2022

[ews-build] Improve error message when Merge-Queue fails to find reviewer #2976

Merged

sammygill removed the merging-blocked Applied to prevent a change from being merged label Aug 4, 2022

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch 2 times, most recently from 941bdbd to a9d682f Compare August 4, 2022 17:14

cdumez added the merge-queue Applied to send a pull request to merge-queue label Aug 4, 2022

pxlcoder reviewed Aug 4, 2022

View reviewed changes

cdumez removed the merge-queue Applied to send a pull request to merge-queue label Aug 4, 2022

sammygill force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from a9d682f to 13e2ff7 Compare August 4, 2022 18:58

pxlcoder added the merge-queue Applied to send a pull request to merge-queue label Aug 4, 2022

webkit-early-warning-system force-pushed the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch from 13e2ff7 to d50f30c Compare August 4, 2022 20:47

webkit-early-warning-system merged commit d50f30c into WebKit:main Aug 4, 2022

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 4, 2022

sammygill deleted the eng/PDF-js-context-menu-should-match-PDFKits-context-menu branch April 19, 2024 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scrolling options to PDFJS context menus. #2417

Add scrolling options to PDFJS context menus. #2417

sammygill commented Jul 14, 2022 •

edited by webkit-commit-queue

nt1m left a comment

sammygill commented Jul 14, 2022

sammygill commented Jul 14, 2022

cdumez commented Jul 21, 2022

sammygill commented Jul 21, 2022

cdumez Jul 21, 2022

sammygill Jul 21, 2022

cdumez Jul 21, 2022

sammygill Jul 21, 2022

cdumez Jul 21, 2022

sammygill Jul 21, 2022

nt1m commented Jul 22, 2022

pxlcoder Aug 4, 2022

webkit-commit-queue commented Aug 4, 2022

webkit-commit-queue commented Aug 4, 2022

pxlcoder Aug 4, 2022

webkit-commit-queue commented Aug 4, 2022

Add scrolling options to PDFJS context menus. #2417

Add scrolling options to PDFJS context menus. #2417

Conversation

sammygill commented Jul 14, 2022 • edited by webkit-commit-queue

nt1m left a comment

Choose a reason for hiding this comment

sammygill commented Jul 14, 2022

sammygill commented Jul 14, 2022

cdumez commented Jul 21, 2022

sammygill commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nt1m commented Jul 22, 2022

Choose a reason for hiding this comment

webkit-commit-queue commented Aug 4, 2022

webkit-commit-queue commented Aug 4, 2022

Choose a reason for hiding this comment

webkit-commit-queue commented Aug 4, 2022

sammygill commented Jul 14, 2022 •

edited by webkit-commit-queue