Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Review: HiddenPdfToolbarItems.md #1467

Merged
merged 5 commits into from Jul 14, 2021
Merged

Conversation

SamuelQZQ
Copy link
Contributor

This is a review for the new HiddenPdfToolbarItems API.

specs/HiddenPdfToolbarItems.md Outdated Show resolved Hide resolved
specs/HiddenPdfToolbarItems.md Outdated Show resolved Hide resolved

# Description
We add a new `HiddenPdfToolbarItems` property in `CoreWebView2Settings`. This API allows end developers to hide buttons on the PDF toolbar.
Currently, this API can hide the _save button_, _save as button_, and _print button_.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I want to hide all buttons, even the ones that I don't know about because they haven't been invented yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a CoreWebView2PdftoolbarItems.All?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have guidance to avoid adding 'All' enum entries because of versioning compat issues. If we want to support a scenario of hiding all items we could add a different boolean property like CoreWebView2Settings.IsPdfToolbarEnabled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, not every button can be hidden. We only allow developers to hide the buttons which we think make sense to be hidden. In the current version, we only allow hiding buttons in the red rectangle.
Snipaste_2021-07-08_14-41-39

Developers can use WebViewSettings.HiddenPdfToolbarItems = ~0 to hide all the hidable button. But actually, it's not equal to disable the toolbar. Because there will be some toolbar buttons still available.

According to this, do you think it's still necessary to add the hide all functionality in this scenario? If we really need this, we need to make every button hidable first.

Copy link
Contributor Author

@SamuelQZQ SamuelQZQ Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add the hide-all API in the future if we need it. And keep the current API without the hide all functionality. @david-risney

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, not every button can be hidden. We only allow developers to hide the buttons which we think make sense to be hidden. In the current version, we only allow hiding buttons in the red rectangle.
Snipaste_2021-07-08_14-41-39

Developers can use WebViewSettings.HiddenPdfToolbarItems = ~0 to hide all the hidable button. But actually, it's not equal to disable the toolbar. Because there will be some toolbar buttons still available.

According to this, do you think it's still necessary to add the hide all functionality in this scenario? If we really need this, we need to make every button hidable first.

I think that instead of you guys deciding "what's best" for the developers use cases (which most/some of the time, it goes against them and sometimes it's just "what the client wants") the best option would be to give the developers the choice to actually do it. If it's already done in some buttons, why not to treat the other buttons the same way? What kind of security implications does it have if you do otherwise? You might not see the use case, but @MikeHillberg surely did, so to me that's a valid use-case too.

Just my two cents here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to use this in a Kiosk mode. Any button that can leak information or allow the user to browse the file system needs to be hidden. This includes the save as button. We also want to hide the print button because that can lead you down a path that could be dangerous.

Once you hide the save as and the print buttons there is little reason to allow the pens that allow marking and highlighting.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like a property to hide the PDF toolbar too, I'm going to implement a PDF viewer in my WPF kiosk application and I really don't want to see anything but the PDF pages and the scroll bar.

Since now it isn't possible to hide the toolbar, do you have any workaround? I saw that the toolbar div has toolbar as id, so you I thought that by executing JavaScript code to set its style to display: none it would do the trick, but document.getElementByIdin this case returns null. Then I saw that the whole PDF was sandboxes inside an embed tag, so I don't know how to make it work.

Do you suggest any workaround? Thanks!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried the HiddenPdfToolbar API. In a separate issue I posted that when I bring up the PDF search box and move my cursor between the X to close the browser and the X to close the search box the application hangs. If I wait long enough, sometimes, the hang goes away. #1889

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-Let us show and hide this toolbar.
-Let us use our own UI to control these functions.

specs/HiddenPdfToolbarItems.md Outdated Show resolved Hide resolved
specs/HiddenPdfToolbarItems.md Outdated Show resolved Hide resolved
@david-risney david-risney added the review completed WebView2 API Proposal that's been reviewed and now needs final update and push label Jul 7, 2021
QZQ and others added 2 commits July 8, 2021 16:13
Co-authored-by: MikeHillberg <18429489+MikeHillberg@users.noreply.github.com>
@SamuelQZQ SamuelQZQ merged commit 7f3fb50 into master Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Proposal Review WebView2 API Proposal for review. review completed WebView2 API Proposal that's been reviewed and now needs final update and push
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants