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-minor] Add support for toggling of Optional Content in the viewer (issue 12096) #12170

Merged
merged 3 commits into from
Aug 30, 2020

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Aug 4, 2020

Please note: Currently blocked by PRs #12163 and #12169 landing first.

Fixes #12096

@Snuffleupagus Snuffleupagus force-pushed the optional-content-viewer-2 branch 6 times, most recently from f2bd346 to 1d19b74 Compare August 5, 2020 08:13
@Snuffleupagus Snuffleupagus force-pushed the optional-content-viewer-2 branch 7 times, most recently from 7bbe338 to 40e792e Compare August 6, 2020 08:25
@Snuffleupagus Snuffleupagus marked this pull request as ready for review August 6, 2020 08:45
@Snuffleupagus Snuffleupagus force-pushed the optional-content-viewer-2 branch 4 times, most recently from 7136336 to 4125dac Compare August 6, 2020 11:50
@Snuffleupagus Snuffleupagus changed the title Add support for toggling of Optional Content in the viewer (issue 12096) [api-minor] Add support for toggling of Optional Content in the viewer (issue 12096) Aug 6, 2020
@Snuffleupagus Snuffleupagus force-pushed the optional-content-viewer-2 branch 9 times, most recently from 3b2fd88 to d5b1668 Compare August 7, 2020 08:25
@Snuffleupagus Snuffleupagus force-pushed the optional-content-viewer-2 branch from d7bce24 to ddb0abf Compare August 28, 2020 10:32
@Snuffleupagus
Copy link
Collaborator Author

Rebased to master to ensure that it still applies cleanly, no actual code changes were made here.

…unning the reference test-suite

This avoids the need to make a round-trip to the worker-thread for *every* single page that's being tested, which should thus be more efficient.
…ontent configuration

The `/Order` array is used to improve the display of Optional Content groups in PDF viewers, and it allows a PDF document to e.g. specify that Optional Content groups should be displayed as a (collapsable) tree-structure rather than as just a list.

Note that not all available Optional Content groups must be present in the `/Order` array, and PDF viewers will often (by default) hide those toggles in the UI.
To allow us to improve the UX around toggling of Optional Content groups, in the default viewer, these hidden-by-default groups are thus appended to the parsed `/Order` array under a *custom* nesting level (with `name == null`).

Finally, the patch also slightly tweaks an `OptionalContentConfig` related JSDoc-comment in the API.
…r (issue 12096)

*Besides, obviously, adding viewer support:* This patch attempts to improve the general API for Optional Content Groups slightly, by adding a couple of new methods for interacting with the (more complex) data structures of `OptionalContentConfig`-instances. (Thus allowing us to mark some of the data as "private", given that it probably shouldn't be manipulated directly.)

By utilizing not just the "raw" Optional Content Groups, but the data from the `/Order` array when available, we can thus display the Layers in a proper tree-structure with collapsible headings for PDF documents that utilizes that feature.

Note that it's possible to reset all Optional Content Groups to their default visibility state, simply by double-clicking on the Layers-button in the sidebar.
(Currently that's indicated in the Layers-button tooltip, which is obviously easy to overlook, however it's probably the best we can do for now without adding more buttons, or even a dropdown-toolbar, to the sidebar.)

Also, the current Layers-button icons are a little rough around the edges, quite literally, but given that the viewer will soon have its UI modernized anyway they hopefully suffice in the meantime.

To give users *full* control of the visibility of the various Optional Content Groups, even those which according to the `/Order` array should not (by default) be toggleable in the UI, this patch will place those under a *custom* heading which:
 - Is collapsed by default, and placed at the bottom of the Layers-tree, to be a bit less obtrusive.
 - Uses a slightly different formatting, compared to the "regular" headings.
 - Is localizable.

Finally, note that the thumbnails are *purposely* always rendered with all Optional Content Groups at their default visibility state, since that seems the most useful and it's also consistent with other viewers.
To ensure that this works as intended, we'll thus disable the `PDFThumbnailView.setImage` functionality when the Optional Content Groups have been changed in the viewer. (This obviously means that we'll re-render thumbnails instead of using the rendered pages. However, this situation ought to be rare enough for this to not really be a problem.)
@Snuffleupagus Snuffleupagus force-pushed the optional-content-viewer-2 branch from ddb0abf to 66aabe3 Compare August 30, 2020 14:28
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/e75ae12bb3f1a9a/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/e75ae12bb3f1a9a/output.txt

Total script time: 3.52 mins

Published

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/8ea2eea647222b8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/639b748863f495a/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/639b748863f495a/output.txt

Total script time: 27.09 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/639b748863f495a/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/8ea2eea647222b8/output.txt

Total script time: 29.91 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/8ea2eea647222b8/reftest-analyzer.html#web=eq.log

@timvandermeij timvandermeij merged commit aa27e7f into mozilla:master Aug 30, 2020
@timvandermeij
Copy link
Contributor

Let's do this. Awesome work!

@Snuffleupagus Snuffleupagus deleted the optional-content-viewer-2 branch August 30, 2020 19:43
@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Sep 1, 2020

Thanks for the reviews here!


Slightly unrelated, but should we perhaps create a new release now?
There's been three months since the last one, and there's been a lot of nice changes landing lately such as optional content, improved form functionality, long-standing font bugs fixed, modernized UI, and more.

I'd also very much suggest that we officially note the next release as the last one with IE 11/Edge (Trident-based) support, to allow moving forward with some much needed clean-up/improvements.

@brendandahl
Copy link
Contributor

Slightly unrelated, but should we perhaps create a new release now?

SGTM

@tbabej
Copy link

tbabej commented Sep 2, 2020

Thanks for the great work here guys!

@timvandermeij
Copy link
Contributor

Good idea. We're now at a point where the functionality has landed and the bugs from testing have been fixed. I'll make the release today.

@Snuffleupagus
Copy link
Collaborator Author

I'll make the release today.

Thank you for doing this!


Just one question here: How about IE 11, couldn't we have explicitly marked the new release as the last one with IE 11 support!?

One example of an IE 11 issue that impacts the Firefox built-in PDF Viewer negatively is the need to ship separate toolbar images for the light/dark themes, thus effectively doubling the size of the image resources for no good reason except for IE 11 support.

@timvandermeij
Copy link
Contributor

I forgot to add that while making the release. I'm not sure what happens if we edit a pre-release on GitHub, if the hooks are re-triggered or not. I'll add this as a question to the NPM issue.

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Sep 5, 2020

I'm not sure what happens if we edit a pre-release on GitHub, if the hooks are re-triggered or not. I'll add this as a question to the NPM issue.

Good point; in the meantime though updating https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support to list IE 11/Edge (Trident-based) as unsupported after v2.6.347 might not be a bad idea.

However, looking at that list it does seem pretty out-of-date in general and it could probably do with some clean-up. Keeping that list even remotely current is (and has been) difficult since we cannot really test all of the mentioned configurations, hence I wonder if there's perhaps a better way to handle things!?
My somewhat naive idea would be to use https://github.com/mozilla/pdf.js#online-demo in order to define supported browsers:

  • If the "Modern browsers"-version works in a particular browser, it's regarded as supported.
  • If the "Older browsers"-version works in a particular browser, it's regarded as partially supported.
  • If none of the demo viewers work in a particular browser, it's simply not supported.

It might be more appropriate to discuss this further in a separate issue, but the above is one idea that attempt to limited the maintenance burden of the list.

@timvandermeij
Copy link
Contributor

I have updated the wiki and added the online demo idea to #11954 which I think is related since I think the required browser features page is in the same way out of date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add UI to control what optional content is visible
7 participants