Display a notification on the `sidebarToggle` button for PDF documents with outline/attachments #7959

Snuffleupagus · 2017-01-16T14:45:45Z

This is something that I've had lying around for quite a while locally, and I figured that I might as well just submit it to see if others think there's value in it!

Fixes #5961
Fixes #7025

Display a notification on the sidebarToggle button for PDF documents with outline/attachments

A longstanding issue with the viewer is that you cannot tell if a PDF document includes an outline and/or attachments without actually opening the sidebar.
This patch contains a suggested solution for that, by displaying an hide-on-interaction notification on the sidebarToggle button (and the relevant sidebar view buttons). Note that this was inspired by e.g. the update notification that is displayed on the menu button in Firefox.

For an initial implementation, I've tried to do this in such a way that the notification isn't too distracting. Without being an UX expert, I don't think that we'd want something too in-your-face, in order to keep the viewer toolbars reasonable clean. (We probably do not want e.g. an entire notification bar in these situations, since that would take up unnecessary screen space and require actions from the user to close.)

However it's certainly possible that the current notification might simply be too inconspicuous to be truly helpful to users, but we could probably iterate on that if the feature itself is deemed useful.
One idea could be to e.g. use a modified tooltip for the sidebarToggle, e.g. "Toggle Sidebar (outline and/or attachments available)", when the notification is used.
Append the contents of FileAttachment annotations to the attachments view of the sidebar, for easier access to the embedded files

Other PDF viewers, e.g. Adobe Reader, seem to append FileAttachments to their attachments views.
One obvious difference in PDF.js is that we cannot append all the annotations on document load, since that would require parsing every page. Despite that, it still seems like a good idea to add FileAttachments, since it's thus possible to access all the various types of attachments from a single place.

Note: With the previous patch we display a notification when a FileAttachment is added to the sidebar, which thus makes appending the contents of these annotations to the sidebar slightly more visible/useful.

This change is

timvandermeij · 2017-01-16T21:27:16Z

Awesome work! I like the idea of this patch. I'm wondering: can we do the green indicator symbol with just CSS instead of using an image?

Snuffleupagus · 2017-01-17T13:47:32Z

Awesome work! I like the idea of this patch.

Thanks for the kind words!

I'm wondering: can we do the green indicator symbol with just CSS instead of using an image?

Really nice idea; I've attempted to implement this, and it seems to work just fine.

timvandermeij

Looks good overall, but I just need a little bit more information to make sure I understand the changes correctly. Thanks!

timvandermeij · 2017-01-17T21:34:21Z

src/display/annotation_layer.js

+    this.content = file.content;
+
+    this.linkService.onFileAttachmentAnnotation({
+      id: stringToPDFString(file.filename),


I'm a bit confused why we don't use this.filename and this.content in these three lines instead. Especially the file name has been sanitized then.

I'm a bit confused why we don't use this.filename and this.content in these three lines instead.

I wanted to make sure that what we (eventually) pass on to pdf_attachment_viewer.js has the exact same format as what PDFDocumentProxy_getAttachments returns, by explicitly using the file attachment data from the API side here, to avoid any possible issues/surprises later on.

Especially the file name has been sanitized then.

Please note that we already need to use getFilenameFromUrl in pdf_attachment_viewer.js, so using it here as well does not help us much and again wouldn't be compatible with what we get from PDFDocumentProxy_getAttachments.

I understand the intention now, thanks!

timvandermeij · 2017-01-17T21:35:56Z

web/pdf_attachment_viewer.js

+          content: content,
+        };
+        this.render({ attachments: attachments, });
+


Nit: let's remove this newline.

Sure, I'll do that when addressing the other comments.

timvandermeij · 2017-01-17T21:40:15Z

web/viewer.css

+  /* Create a filled circle, with a diameter of 10 pixels, using only CSS: */
+  content: '';
+  background-color: #8BE651;
+  height: 10px;


I think it would look better if the diameter would be a little bit smaller, for example 8px (or another value that you find appropriate). It makes the notification a bit more subtle while still being visible enough. Do you agree?

I basically just picked a size, more or less arbitrarily, that wouldn't be too large while still being easily visible.
If we decrease the size, my only worry is that the notification could become too small to be easily noticeable. However, using a slightly brighter color would probably help with that.

Which of the following ones would you prefer?
https://www.dropbox.com/s/prv8vyuxagf5vln/notification-sizes.png?dl=0

Also, do you think that we should use a different tooltip for the sidebarToggle button when outline/attachments exists?
E.g. adding a new toggle_sidebar_notification.title=Toggle Sidebar (document has outline/attachments) localization string and using that here.

I think 9px is perfect. It's very nitpicky, but I feel 8px is too small and 10px is slightly too large, now that they're all visualized. I don't think we should change the tooltip. The visual notification is clear enough to show the user that attachments/outlines exist, and people generally don't know about/don't notice tooltips (as we have seen before with questions about outline tree toggling).

The visual notification is clear enough to show the user that attachments/outlines exist,

Hopefully you're right about that, since there's "similar" notifications used in some places in the Firefox UI (but please see below).

and people generally don't know about/don't notice tooltips (as we have seen before with questions about outline tree toggling).

Even though it's probably accurate that most people don't care about tooltips, wasn't the case with the outline you refer to somewhat opposite?
Provided that it's issue #7713 and PR #7717 you're thinking of here: the issue (as I understood it) was that there were no tooltip explaining the behavior, which thus necessitated that PR.

One of the reasons that I suggested a new tooltip here[1], is to hopefully avoid future followups once someone complains that the sidebar notification isn't clear enough as is.

[1] Not to replace the current sidebar tooltip, but rather supplement it.

You're right, I mixed that up. Personally I feel that the tooltip there was only useful because it was about an instruction that could not be displayed visually. Here, however, we have the visual notification. In my opnion, that should be enough, but I'm not totally against adding a tooltip, so if you think it would be more clear to add the tooltip as well, I'm fine with that.

I've updated the size to 9px and changed the colour slightly (such that it's now using a value found in http://firefoxux.github.io/StyleGuide/#/colours).

Also, after going back-and-forth a bit on the tooltip, I figured that it probably can't hurt to add one.
My line of reasoning is that while we know the context here (i.e. from this PR), a user that sees the notification may find the tooltip helpful to explain what the notification is about.
However, it's easy to change or even remove the tooltip, if we decide to do so later on.
@timvandermeij Does the implementation seem reasonable here, in your opinion?

timvandermeij · 2017-01-17T21:41:05Z

web/pdf_sidebar.js

@@ -104,7 +106,7 @@ var PDFSidebar = (function PDFSidebarClosure() {
    reset: function PDFSidebar_reset() {
      this.isInitialViewSet = false;

-      this.close();
+      this._hideUINotification(null);


Is it intended that the close() call is removed here? If so, what is the rationale for that?

This is just fallout from testing, since I wanted to keep the sidebar open to be able to check that all notifications were being removed on reset as intended.

However, I'm not sure that we actually must close the sidebar when resetting it; so I'm not sure if we need to put the this.close(); call back!?

I think it's there for the situation where you have opened a document with attachments/outlines and open a new document without attachments/outlines. You may be right though, because if there are attachments and render is called to add a new one (leading to reset being called), we do not want it to close, which I think will happen if close is put back. Could you test that to be sure that both scenarios still work (adding a new attachment without closing the sidebar, and opening a document without attachments/outlines)?

I'm sorry, but I'm slightly confused by #7959 (comment).

The only place which calls PDFSidebar_reset is the close function in app.js. Hence the only effect of removing the this.close() call from here, i.e. inside of pdf_sidebar.js, is that the sidebar will not be force closed when opening a new document in the default viewer. So this change only applies to an edge-case really.

Please note that the PDFOutlineViewer_reset/PDFAttachmentViewer_reset methods does not reset the sidebar itself, but rather only the relevant view.

Hopefully this answers your questions! But please ask again if I misunderstood what you meant.

You're right about that. I'm fine with the removal of this.close() as long as no negative side-effects occur, such as an open sidebar where the view is not reset when a document is opened that has no outline/attachments anymore.

I'm fine with the removal of this.close() as long as no negative side-effects occur, such as an open sidebar where the view is not reset when a document is opened that has no outline/attachments anymore.

On reset, we're already switching to the Thumbnail View (this logic is not changed in the patch); see https://github.com/mozilla/pdf.js/blob/master/web/pdf_sidebar.js#L108.

Once the Outline/Attachments have been loaded, we will also switch to the Thumbnail View if the other views are empty; see https://github.com/mozilla/pdf.js/blob/master/web/pdf_sidebar.js#L346-L349 and https://github.com/mozilla/pdf.js/blob/master/web/pdf_sidebar.js#L355-L358.

timvandermeij · 2017-01-17T21:41:59Z

web/pdf_attachment_viewer.js

      this.attachments = null;

      var container = this.container;
      while (container.firstChild) {
        container.removeChild(container.firstChild);
      }
+
+      if (!keepRenderedCapability) {


Could you elaborate a bit on why we need the keepRenderedCapability option? It's not entirely clear to me what its use is.

Just to make sure that we're on the same page, a bit of background first:
I wanted to ensure that what PDFDocumentProxy_getAttachments returns always takes precedence over any file attachments, to not change the current behavior. This is why the _renderedCapability is used, to ensure that we wait for the "regular" attachments to load before attempting to add any existing file attachments.

Now, when resetting PDFAttachmentViewer we generally want to replace the _renderedCapability to avoid the situation where opening a new document (in the viewer) could lead to file attachments from the previous document being applied to the new one (given the async nature of Promises).
However, since the new _appendAttachments method calls render which will reset the attachment view (see pdf_attachment_viewer.js#L96) to ensure that everything is rendered correctly and in alphabetic order, replacing the _rendererCapability here could cause pending _appendAttachments calls to be incorrectly skipped.
Hence why we need to keep the _renderedCapability in this case.

Since this isn't an option that anyone should pass in when using PDFAttachmentViewer.reset(); from the outside (as in e.g.

pdf.js/web/app.js

Line 595 in a917443

close: function pdfViewClose() {

), I didn't want to add JSDocs for it. However, I'll try to add some sort of inline comment here, to briefly explain why it's there.

Sorry, this became a bit long. Hopefully it's enough to answer your question!

Edit: It just occurred to me that keepRenderedCapability should probably be an option on render instead, and not unconditionally set to true when calling reset from within render.

Thanks, I now understand why we need to have this. Indeed, an inline comment (and no JSDoc comment) would be helpful to avoid any confusion. I'm also fine with moving the option to render instead.

Indeed, an inline comment (and no JSDoc comment) would be helpful to avoid any confusion. I'm also fine with moving the option to render instead.

Addressed in the latest version of the PR; please do let me know if the comment isn't clear enough!

Snuffleupagus · 2017-01-19T08:00:58Z

web/pdf_attachment_viewer.js

+          attachments = Object.create(null);
+        } else {
+          for (var name in attachments) {
+            if (id === attachments[name]) {


This condition doesn't actually make sense, it should obviously be if (id === name) { instead.

timvandermeij

This version of the PR is much better. I'm currently lacking time a bit to review the PR completely, but here are some drive-by comments. I'll hopefully have time during the weekend to complete the review.

timvandermeij · 2017-01-25T22:31:21Z

l10n/en-US/viewer.properties

@@ -101,6 +101,7 @@ print_progress_close=Cancel
 # (the _label strings are alt text for the buttons, the .title strings are
 # tooltips)
 toggle_sidebar.title=Toggle Sidebar
+toggle_sidebar_notification.title=Toggle Sidebar (document contains outline/attachment)


I think it's better to use the plural form attachments here.

D'oh; would you believe me if I said that I always intended to use the plural form (i.e. attachments), and even thought that I actually did :-)
Somehow though, when writing this the s went missing, probably because outline should be in singular.
@timvandermeij Very good catch, I'll address this once you've had time to finish the review (no rush though)!

timvandermeij · 2017-01-25T22:31:56Z

web/pdf_sidebar.js

@@ -314,6 +321,67 @@ var PDFSidebar = (function PDFSidebarClosure() {
    /**
     * @private
     */
+    _showUINotification: function (view) {
+      this.toggleButton.title = mozL10n.get('toggle_sidebar_notification.title',
+        null, 'Toggle Sidebar (document contains outline/attachment)');


The comment about attachments applies here as well.

timvandermeij

Looks good! r=me with the previously added comments addressed and passing tests.

Snuffleupagus · 2017-01-30T13:26:02Z

The review comments have now been addressed; @timvandermeij thanks for many good review comments!
Note that I added one new thing here: a PDFSidebar parameter to disable the notification(s). I don't think that we really need to make it a regular viewer preference, but I figured that it cannot hurt to have a simple way to disable this feature (e.g. from within app.js).

/botio-linux preview

…s with outline/attachments A longstanding issue with the viewer is that you cannot tell if a PDF document includes an outline and/or attachments without actually opening the sidebar. This patch contains a suggested solution for that, by displaying an hide-on-interaction notification on the `sidebarToggle` button (and the relevant sidebar view buttons). Note that this was inspired by e.g. the update notification that is displayed on the menu button in Firefox. For an initial implementation, I've tried to do this in such a way that the notification isn't too distracting. Without being an UX expert, I don't think that we'd want something too in-your-face, in order to keep the viewer toolbars reasonable clean. (We probably do *not* want e.g. an entire notification bar in these situations, since that would take up unnecessary screen space and require actions from the user to close.) However it's certainly possible that the current notification might simply be *too* inconspicuous to be truly helpful to users, but we could probably iterate on that if the feature itself is deemed useful.

timvandermeij · 2017-01-30T22:51:52Z

/botio-linux preview

pdfjsbot · 2017-01-30T22:51:53Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/fa9737c8bdc1a3d/output.txt

pdfjsbot · 2017-01-30T22:54:06Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/fa9737c8bdc1a3d/output.txt

Total script time: 2.22 mins

Published

timvandermeij · 2017-01-30T22:55:55Z

/botio test

pdfjsbot · 2017-01-30T22:55:55Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/697dab1dc43bdb7/output.txt

pdfjsbot · 2017-01-30T22:55:55Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/95d46f93b30df6a/output.txt

pdfjsbot · 2017-01-30T23:23:17Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/697dab1dc43bdb7/output.txt

Total script time: 27.36 mins

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

Image differences available at: http://107.21.233.14:8877/697dab1dc43bdb7/reftest-analyzer.html#web=eq.log

pdfjsbot · 2017-01-30T23:23:29Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/95d46f93b30df6a/output.txt

Total script time: 27.56 mins

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

Image differences available at: http://107.22.172.223:8877/95d46f93b30df6a/reftest-analyzer.html#web=eq.log

timvandermeij · 2017-01-31T21:18:16Z

The bots fail at annotation-fileattachment.pdf. I don't think that's coincidence, but opening that file manually in the viewer works just fine. Any ideas as to why this fails here?

…s view of the sidebar, for easier access to the embedded files Other PDF viewers, e.g. Adobe Reader, seem to append `FileAttachment`s to their attachments views. One obvious difference in PDF.js is that we cannot append all the annotations on document load, since that would require parsing *every* page. Despite that, it still seems like a good idea to add `FileAttachment`s, since it's thus possible to access all the various types of attachments from a single place. *Note:* With the previous patch we display a notification when a `FileAttachment` is added to the sidebar, which thus makes appending the contents of these annotations to the sidebar slightly more visible/useful.

Snuffleupagus · 2017-01-31T21:36:56Z

The bots fail at annotation-fileattachment.pdf. I don't think that's coincidence, but opening that file manually in the viewer works just fine. Any ideas as to why this fails here?

Normally I take great care to run all applicable tests locally, but here I forgot to do that since I didn't think that any of changes in the src/ folder would matter. Obviously I was wrong, since the LinkServiceMock used by the test-suite was missing (now) required methods.
The best solution would probably be if we could use the SimpleLinkService in the tests, but for now I've amended the LinkServiceMock in test/driver.js with all the missing methods.

@timvandermeij Thanks for pointing out exactly where the tests failed, since that helped me to very quickly pinpoint the exact location of the problem.

Snuffleupagus · 2017-01-31T22:04:15Z

/botio test

pdfjsbot · 2017-01-31T22:04:15Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/d7d7b26011f7da9/output.txt

pdfjsbot · 2017-01-31T22:04:16Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/7b48db242d1de36/output.txt

pdfjsbot · 2017-01-31T22:29:45Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/d7d7b26011f7da9/output.txt

Total script time: 25.49 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2017-01-31T22:29:46Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/7b48db242d1de36/output.txt

Total script time: 25.50 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2017-02-01T21:59:52Z

Nice work, thanks!

Snuffleupagus · 2017-02-02T11:49:24Z

@timvandermeij As always: Thank you for reviewing this, and other of my PRs!

…ations Display a notification on the `sidebarToggle` button for PDF documents with outline/attachments

Snuffleupagus added ux viewer labels Jan 16, 2017

timvandermeij reviewed Jan 17, 2017

View reviewed changes

Snuffleupagus commented Jan 19, 2017

View reviewed changes

Snuffleupagus added the 4-work-in-progress label Jan 19, 2017

timvandermeij reviewed Jan 25, 2017

View reviewed changes

timvandermeij approved these changes Jan 29, 2017

View reviewed changes

timvandermeij removed the 4-work-in-progress label Jan 29, 2017

timvandermeij merged commit 37ecb8e into mozilla:master Feb 1, 2017

Snuffleupagus deleted the PDFSidebar-notifications branch February 2, 2017 11:44

Snuffleupagus mentioned this pull request Jul 1, 2017

Change var to let, and use object destructuring, in a couple of previously class converted web/*.js files #8594

Merged

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#7959 from Snuffleupagus/PDFSidebar-notific…

c7f3d30

…ations Display a notification on the `sidebarToggle` button for PDF documents with outline/attachments

awesome-manuel mentioned this pull request Jul 9, 2020

Feature request: Embed files parallax/jsPDF#2606

Closed

Snuffleupagus mentioned this pull request Jan 18, 2021

Only display a notification on the sidebarToggle-button, and not the individual view-buttons (PR 7959 follow-up) #12871

Merged

Display a notification on the sidebarToggle button for PDF documents with outline/attachments #7959

Display a notification on the sidebarToggle button for PDF documents with outline/attachments #7959

Conversation

Snuffleupagus commented Jan 16, 2017 • edited by timvandermeij Loading

timvandermeij commented Jan 16, 2017

Snuffleupagus commented Jan 17, 2017 • edited by timvandermeij Loading

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

timvandermeij Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 18, 2017 • edited Loading

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

timvandermeij left a 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

timvandermeij left a comment

Choose a reason for hiding this comment

Snuffleupagus commented Jan 30, 2017 • edited Loading

timvandermeij commented Jan 30, 2017

pdfjsbot commented Jan 30, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 30, 2017

From: Bot.io (Linux)

Success

Published

timvandermeij commented Jan 30, 2017

pdfjsbot commented Jan 30, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 30, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jan 30, 2017

From: Bot.io (Linux)

Failed

pdfjsbot commented Jan 30, 2017

From: Bot.io (Windows)

Failed

timvandermeij commented Jan 31, 2017

Snuffleupagus commented Jan 31, 2017

Snuffleupagus commented Jan 31, 2017

pdfjsbot commented Jan 31, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 31, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jan 31, 2017

Display a notification on the `sidebarToggle` button for PDF documents with outline/attachments #7959

Display a notification on the `sidebarToggle` button for PDF documents with outline/attachments #7959

Snuffleupagus commented Jan 16, 2017 •

edited by timvandermeij

Loading

Snuffleupagus commented Jan 17, 2017 •

edited by timvandermeij

Loading

Snuffleupagus Jan 18, 2017 •

edited

Loading

timvandermeij Jan 17, 2017 •

edited

Loading

timvandermeij Jan 17, 2017 •

edited

Loading

timvandermeij Jan 18, 2017 •

edited

Loading

Snuffleupagus Jan 19, 2017 •

edited

Loading

Snuffleupagus Jan 25, 2017 •

edited

Loading

Snuffleupagus Jan 18, 2017 •

edited

Loading

timvandermeij Jan 18, 2017 •

edited

Loading

Snuffleupagus Jan 19, 2017 •

edited

Loading

Snuffleupagus Jan 25, 2017 •

edited

Loading

Snuffleupagus Jan 18, 2017 •

edited

Loading

Snuffleupagus commented Jan 30, 2017 •

edited

Loading