Allow outline to be collapsed / shown via +/-. #6242

Rob--W · 2015-07-21T18:16:09Z

I was trying to navigating through a PDF with a huge outline (http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/pdf_reference_1-7.pdf), and needed a way to toggle a subtree under an outline item. Unfortunately this feature was not yet available, so I decided to implement it.

This PR adds a '-' before every outline item that has subitems.
Clicking on the '-' to collapses all subitems under that item (which turns the '-' in a '+').
Shift + click expands/collapses all descendant items under that tree.
Double-clicking on the "Show Document Outline" button in the toolbar expands/collapses all outline items.

To test: Preview this PR and check whether the four listed features works for the given PDF file. And for a good measure, double-clicking on the "Show Document Outline" button does nothing for a document without an outline (no errors, just nothing).

timvandermeij · 2015-07-21T18:41:24Z

Funtionality-wise this appears to be the same as #3753, but code-wise it looks much cleaner to me. Also, that previous PR has not been touched for a while.

Some initial feedback: I do think the +/- signs should be the same color as the text (white) to make them stand out, as they are currently not very visible.

@Snuffleupagus What do you think? Is there funtionality in your PR that this PR does not have?

Snuffleupagus · 2015-07-21T18:43:24Z

I suggested something similar in PR #3753, but I don't think that consensus was reached regarding if we wanted this feature or not (which is why I stopped working on it).

timvandermeij · 2015-07-21T18:49:47Z

I think it is a useful feature (as I have also been working with the spec a lot lately and this was bothering me too), from #3753 (comment) I sense that @brendandahl likes the idea and there was someone else in that PR that wanted it. Now @Rob--W would like to have it too. Only @yurydelendik was unsure about the feature.

Rob--W · 2015-07-21T20:27:29Z

@timvandermeij I've added a grey-ish color to the +/- (actually, white with 50% transparency).

timvandermeij · 2015-07-21T20:39:40Z

Yes, it looks much better this way. I also like how the subitems are highlighted when you hover over the +/- sign, and how this is done with pure CSS and no images. A few additional remarks:

Could you add cursor:pointer; to the +/- signs? That makes it more clear that the user can click it.
Personally I would leave out the Shift+click and double click actions as they are quite hidden and not really necessary (at least for a first version). For example, it does not really make sense to me that double clicking the outline button will uncollapse all items, and it might even be confusing if it is done on accident. If we do want those features, we should make them more visible in the UI, but that can be done in a follow-up PR. It would be nice to get the basic functionality without too much code/UI changes first, because from past experiences we know that it is better to add new functionality to the UI more slowly (and to make review easier as we do not have automated tests for the viewer part).

Aside from that I really like this PR. I will play with it more the coming days.

Edit: I would also increase the size of the +/- sign somewhat to make it easier to recognize and to be able to click it more easily.

timvandermeij · 2015-07-21T21:02:02Z

web/pdf_outline_view.js

+    _addToggleButton: function PDFOutlineView_addToggleButton(div) {
+      var toggler = document.createElement('div');
+      toggler.className = 'outlineItemToggler';
+      toggler.onclick = function(event) {


Let's use toggler.addEventListener('click', function() { ... }); here as we use addEventListener almost everywhere in the codebase.

Almost everywhere, except in this file:

pdf.js/web/pdf_outline_view.js

Line 66 in 8623421

element.onclick = function goToDestination(e) {

OK, we can keep it as-is then to at least keep it consistent with that file.

There are more examples, so I think this is fine; see e.g.

https://github.com/mozilla/pdf.js/blob/master/web/annotations_layer_builder.js#L53

https://github.com/mozilla/pdf.js/blob/master/web/annotations_layer_builder.js#L66

https://github.com/mozilla/pdf.js/blob/master/web/pdf_attachment_view.js#L65

Snuffleupagus · 2015-07-21T21:04:07Z

I've just closed PR #3753, since I think that this one is probably a lot more likely to be accepted.

A couple of questions/ideas:

Would it be possible to make the +/- slightly larger (and/or bolder), since even with the colour change they are still a bit hard to spot?
Another idea is to ask @shorlander for +/- icons, and use that instead.
Also, since the effective width for each outlineItem is now slightly reduced (given that the +/- requires some horizontal space), could we perhaps reduce the padding somewhat?

I.e. could we change viewer.css#L1223-L1225 to something like this (and similarly for rtl):

 html[dir='ltr'] .outlineItem > a {
-  padding: 2px 0 5px 10px;
+  padding: 2px 0 5px 5px;
 }

Rob--W · 2015-07-21T21:15:30Z

@timvandermeij

Could you add cursor:pointer; to the +/- signs? That makes it more clear that the user can click it.

The + / - and getting a different color upon hover offers enough affordance in my opinion. I intentionally used a cursor different from the link, so that it's easier to notice that you're no longer hovering over the link.

Personally I would leave out the Shift+click and double click actions as they are quite hidden and not really necessary (at least for a first version). For example, it does not really make sense to me that double clicking the outline button will uncollapse all items, and it might even be confusing if it is done on accident.

I do want an option to toggle the whole tree, since clicking on the items one by one is a bit clumsy. In UX, double-clicking is often used to be a stronger version of the single-click. Clicking once shows the outline, and clicking again does nothing at the moment. That's why I used double-click, it shows the outline again (fully expands outline) or collapses the outline (when you double-click again).
Double-clicking for the +/- icon is inconvenient, because a single click always does something, so double-clicking is a quick show/hide. Attaching "collapse/expand" all to double-click on +/- is very confusing, that's why I opted for shift-click. There is no standard for collapse/expand, but some others seem to also agree that ctrl or shift in addition to click makes sense as a trigger (https://ux.stackexchange.com/questions/12316/conventional-keyboard-modifier-for-expanding-collapsing-hierarchical-tree-view).

If we do want those features, we should make them more visible in the UI, but that can be done in a follow-up PR. It would be nice to get the basic functionality without too much code/UI changes first, because from past experiences we know that it is better to add new functionality to the UI more slowly (and to make review easier as we do not have automated tests for the viewer part).

After merging, I plan on adding documentation to the wiki (https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#user-content-what-are-the-pdfjs-keyboard-shortcuts), so it'll be discoverable by users. None of the current shortcuts are discoverable from the UI, so if we're going to e.g. change the tooltip to add the shortcut, then we'd better do that for every item.

@Snuffleupagus & @timvandermeij

I'll play with the styles and follow up soon.

timvandermeij · 2015-07-21T21:26:34Z

Actually, those are some really good points! The functionality and the choices that inspired it are fine then, as long as we document them properly in the wiki. What remains for me then is to make the click area larger because I find that I often need to click twice because the signs are small, but that will probably be fixed automatically if you increase the size of the signs.

Nice work. I'm curious to see where this is going.

Rob--W · 2015-07-21T21:30:39Z

@timvandermeij The icon (although not clearly visible as such) is currently 15x15 pixels. Do you think that this needs to be bigger? Edit: I personally think that a bit larger is better than 15x15.

left (15x20) right (20x20)

Snuffleupagus · 2015-07-21T21:48:56Z

The +/- icons, nor their clickable area, aren't particularly big in e.g. Adobe Reader, see below.
I think it would be unfortunate to increase (particularly) the width of the +/- too much, since they are then "stealing" vertical space that would probably benefit the text of the outlineItems more.

Rob--W · 2015-07-21T21:49:08Z

Updated PR with bigger icons and smaller padding. WDYT?

diff --git a/web/viewer.css b/web/viewer.css
index 6091c94..875ff36 100644
--- a/web/viewer.css
+++ b/web/viewer.css
@@ -1223,7 +1223,7 @@ html[dir='rtl'] .outlineItem > .outlineItems {
 }

 html[dir='ltr'] .outlineItem > a {
-  padding: 2px 0 5px 10px;
+  padding: 2px 0 5px 4px;
 }
 html[dir='ltr'] .attachmentsItem > button {
   padding: 2px 0 3px 7px;
@@ -1231,7 +1231,7 @@ html[dir='ltr'] .attachmentsItem > button {
 }

 html[dir='rtl'] .outlineItem > a {
-  padding: 2px 10px 5px 0;
+  padding: 2px 4px 5px 0;
 }
 html[dir='rtl'] .attachmentsItem > button {
   padding: 2px 7px 3px 0;
@@ -1246,10 +1246,14 @@ html[dir='rtl'] .attachmentsItem > button {
 }
 .outlineItemToggler::before {
   display: block;
-  content: '-';
+  content: '\02012'; /* figure dash */
   position: absolute;
-  width: 15px;
-  height: 15px;
+  width: 20px;
+  height: 20px;
+  line-height: 20px;
+  text-align: center;
+  font-size: 18px;
+  font-weight: bold;
 }
 .outlineItemToggler.outlineItemsHidden::before {
   content: '+';

Rob--W · 2015-07-21T21:50:13Z

@Snuffleupagus The left margin is already 20px, so having a 20px wide button won't cause a loss in estate.

Snuffleupagus · 2015-07-21T22:12:56Z

Yes, that does look better!
One thing that bothers me though, is that the +/- isn't centred properly vertically. They end up way too low, in my opinon. It appears that they are almost placed at the outlineItem text baseline, as illustrated here (screen-shot taken in Firefox 42):

Rob--W · 2015-07-21T22:23:07Z

@Snuffleupagus I've picked a different pairs of (matching) (unicode) characters. How does that look?

Snuffleupagus · 2015-07-22T07:30:25Z

I've picked a different pairs of (matching) (unicode) characters. How does that look?

That actually looks worse than the previous version, i.e. #6242 (comment) and #6242 (comment), see:

The only thing "wrong" with the previous version was the vertical placement of the +/-. Would just reducing the line-height, in #6242 (comment), have fixed the vertical alignment issues of the +/-?

Rob--W · 2015-07-22T08:08:46Z

@Snuffleupagus What part "looks worse"? The horizontal line is correctly aligned, isn't it? If the icon is too big, we can decrease its font size.

The line height is chosen to be equal to the height, so that the glyph is vertically centered. The appearance is up to the font designer and font renderer, but I think that it's easier to get consistent and reliable results with the new unicode fonts (HEAVY MINUS SIGN & HEAVY PLUS SIGN) than the previous options (FIGURE DASH and PLUS).

Snuffleupagus · 2015-07-22T08:26:34Z

What part "looks worse"? The horizontal line is correctly aligned, isn't it? If the icon is too big, we can decrease its font size.

I was referring to the "huge" size of the icons. Sorry, I should have been more clear!

Rob--W · 2015-07-22T08:29:45Z

@Snuffleupagus I adjusted the font size. Take another look ;)

Snuffleupagus · 2015-07-22T11:18:49Z

That looks better!
I've tested this in both Firefox and Chrome (on Windows), and I've got one small additional suggestion. Now that the patch uses "special" Unicode characters, can we remove font-weight: bold;?
Because currently the vertical "bar" of the "+" sign look a bit heavy, in my opinion, compare:

Snuffleupagus · 2015-07-22T11:20:25Z

web/pdf_outline_view.js

+    _toggleOutlineItem: function PDFOutlineView_toggleOutlineItem(root, show) {
+      this.lastToggleIsShow = show;
+      var togglers = root.querySelectorAll('.outlineItemToggler');
+      for (var i = 0; i < togglers.length; ++i) {


Nit: cache togglers.length, e.g.

for (var i = 0, ii = togglers.length; i < ii; ++i) {

Done.

Just out of curiousity, why did you want this change? If mainly for consistency with the code base, then I agree. If for performance, then I don't (introducing an extra variable for the length of a non-live NodeList is not going to have any measurable impact on the performance.).

I think that caching might have helped in older browsers, which is probably why we use it even though it's not really necessary today.
Considering that we currently use this pattern "everywhere" in the code-base, consistency is nice :)

Rob--W · 2015-07-22T12:46:35Z

@Snuffleupagus I've removed the bold font weight, and also increased the font size from 13 to 15px to avoid the appearance of a too tiny icon.

timvandermeij · 2015-07-22T19:24:13Z

With these new characters and styles it looks a lot better. The icons are still quite large for me, and with the developer tools I have found that a font size of 11px seems the most reasonable to me. It currently looks like https://cloud.githubusercontent.com/assets/2692120/8820162/503cd984-3053-11e5-88a6-bd139a94305c.png for me, only without the bold style. They should be smaller in my opinion and 11px makes them look just big enough to click, yet not too invasive.

Rob--W · 2015-07-22T19:43:32Z

@timvandermeij It depends on the font, I guess. Here's how the +/- looks to me (Chromium, Linux). Top = 15px, bottom = 11px. What about 13px? That's the font size of the label next to the +/-. In the short term, a +/- might do, but perhaps it's better to ask Stephen Horlander for expand/collapse icons (side note, we already have +/- icons for zoom in/out.).

timvandermeij · 2015-07-22T20:02:17Z

Yes, it seems to depend on the font. Let's do 13px then, that still looks good to me. I actually think that this solution is nicer than using images as this automatically looks good on hidpi screens. I'm also fine with icons though. The fastest way is to send Stephen an e-mail at shorlander@mozilla.com, as he is not very active on GitHub, and ask for his opinion and icons.

timvandermeij · 2015-07-22T20:06:51Z

Also, if you e-mail Stephen, remember to ask for @2x assets too, as those are used for hidpi screens. See https://github.com/mozilla/pdf.js/tree/master/web/images for examples.

Rob--W · 2015-07-22T20:09:40Z

@timvandermeij I've switched from 15px to 13px in the latest version of the PR.

Why don't we use SVG instead of PNG for the images? That looks good on any screen, at any zoom level.

timvandermeij · 2015-07-22T20:14:42Z

/botio-linux preview

pdfjsbot · 2015-07-22T20:14:43Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2015-07-22T20:15:21Z

From: Bot.io (Linux)

Success

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

Total script time: 0.63 mins

Published

timvandermeij · 2015-07-22T20:17:04Z

@Rob--W That would be ideal, but that has to be done in a follow-up PR as all icons would have to be remade in SVG format by the UX team.

I'll ask the UX team on IRC what they think about this PR and whether or not we should use icons, and if they can provide those. That might be easier than e-mailing.

Rob--W · 2015-08-08T21:51:46Z

I haven't received an icon from anyone. On IRC someone (@brendandahl or @yurydelendik, I don't remember) suggested to use triangles (like Firefox's devtools).

Which icon shall we pick?

timvandermeij · 2015-08-09T11:43:40Z

You can ping shorlander on irc.mozilla.org at #ux. IIRC, he has been working on those icons and also thought about using triangles.

shorlander · 2015-08-21T15:25:32Z

Sorry it took so long to get to this. I would use the disclosure arrows instead of a + and -

Rob--W · 2015-08-22T15:17:45Z

Thanks for the icons @shorlander. I have updated the PR to use the given icons instead of +/-.

- This commit adds a '>' before every outline item that has subitems. - Click on the '>' to collapse all subitems under that item (which turns the '>' in a 'v'). - Shift + click expands/collapses all descendant items under that tree. - Double-clicking on the "Show Document Outline" button in the toolbar expands/collapses all outline items.

Rob--W · 2015-08-26T21:46:05Z

@yurydelendik Could you review and merge this PR? See the initial message for the functionality (read "+" / "-" as "expand-icon" / "collapse-icon").

@timvandermeij and @Snuffleupagus are definitely positive towards this feature, and we have icons from shorlander. I guess that the PR can be merged if your concerns have been resolved (#3753 (comment)).

Once merged, I will add documentation to the wiki.

yurydelendik · 2015-08-27T13:35:39Z

/botio-windows preview

pdfjsbot · 2015-08-27T13:35:39Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/2de28aa9aed49f7/output.txt

pdfjsbot · 2015-08-27T13:36:44Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/2de28aa9aed49f7/output.txt

Total script time: 1.07 mins

Published

Allow outline to be collapsed / shown via +/-.

yurydelendik · 2015-08-27T13:41:40Z

Thank you for the patch.

Rob--W added viewer ux labels Jul 21, 2015

Rob--W force-pushed the outline-collapsible branch from 1c74aea to b3fb8b4 Compare July 21, 2015 20:25

timvandermeij mentioned this pull request Jul 21, 2015

Enable collapsing/expanding of Outline Items #3753

Closed

timvandermeij reviewed Jul 21, 2015
View reviewed changes

Rob--W force-pushed the outline-collapsible branch from b3fb8b4 to e9d6d63 Compare July 21, 2015 21:48

Rob--W force-pushed the outline-collapsible branch from e9d6d63 to 605f1e5 Compare July 21, 2015 22:22

Rob--W force-pushed the outline-collapsible branch from 605f1e5 to ba96b5f Compare July 22, 2015 08:29

Snuffleupagus reviewed Jul 22, 2015
View reviewed changes

Rob--W force-pushed the outline-collapsible branch from ba96b5f to b96f0f3 Compare July 22, 2015 12:44

Rob--W force-pushed the outline-collapsible branch from b96f0f3 to b5eb839 Compare July 22, 2015 20:05

timvandermeij mentioned this pull request Jul 22, 2015

Replace PNG assets with SVG assets #6252

Closed

Rob--W force-pushed the outline-collapsible branch from b5eb839 to 105b55f Compare August 22, 2015 15:17

Rob--W force-pushed the outline-collapsible branch from 105b55f to 7c99224 Compare August 22, 2015 16:49

Rob--W assigned yurydelendik Aug 26, 2015

yurydelendik added a commit that referenced this pull request Aug 27, 2015

Merge pull request #6242 from Rob--W/outline-collapsible

70fd360

Allow outline to be collapsed / shown via +/-.

yurydelendik merged commit 70fd360 into mozilla:master Aug 27, 2015

Snuffleupagus mentioned this pull request Aug 27, 2015

Use the Retina assets for outlineItemToggler when in HiDPI mode #6388

Merged

This was referenced Sep 6, 2015

Adjust the min-width of outline items and use more consistent border-radius (PR 6242 follow-up) #6421

Merged

Wrong direction used for the collapsed outlineItem icon in RTL locales? #6422

Closed

Lekensteyn pushed a commit to Lekensteyn/pdf.js-wiki that referenced this pull request Mar 7, 2019

Document outline controls (introduced in mozilla/pdf.js#6242).

327c3fb

Allow outline to be collapsed / shown via +/-. #6242

Allow outline to be collapsed / shown via +/-. #6242

Conversation

Rob--W commented Jul 21, 2015

timvandermeij commented Jul 21, 2015

Snuffleupagus commented Jul 21, 2015

timvandermeij commented Jul 21, 2015

Rob--W commented Jul 21, 2015

timvandermeij commented Jul 21, 2015

timvandermeij Jul 21, 2015

Choose a reason for hiding this comment

Rob--W Jul 21, 2015

Choose a reason for hiding this comment

timvandermeij Jul 21, 2015

Choose a reason for hiding this comment

Snuffleupagus Jul 21, 2015

Choose a reason for hiding this comment

Snuffleupagus commented Jul 21, 2015

Rob--W commented Jul 21, 2015

timvandermeij commented Jul 21, 2015

Rob--W commented Jul 21, 2015

Snuffleupagus commented Jul 21, 2015

Rob--W commented Jul 21, 2015

Rob--W commented Jul 21, 2015

Snuffleupagus commented Jul 21, 2015

Rob--W commented Jul 21, 2015

Snuffleupagus commented Jul 22, 2015

Rob--W commented Jul 22, 2015

Snuffleupagus commented Jul 22, 2015

Rob--W commented Jul 22, 2015

Snuffleupagus commented Jul 22, 2015

Snuffleupagus Jul 22, 2015

Choose a reason for hiding this comment

Rob--W Jul 22, 2015

Choose a reason for hiding this comment

Snuffleupagus Jul 22, 2015

Choose a reason for hiding this comment

Rob--W commented Jul 22, 2015

timvandermeij commented Jul 22, 2015

Rob--W commented Jul 22, 2015

timvandermeij commented Jul 22, 2015

timvandermeij commented Jul 22, 2015

Rob--W commented Jul 22, 2015

timvandermeij commented Jul 22, 2015

pdfjsbot commented Jul 22, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Jul 22, 2015

From: Bot.io (Linux)

Success

Published

timvandermeij commented Jul 22, 2015

Rob--W commented Aug 8, 2015

timvandermeij commented Aug 9, 2015

shorlander commented Aug 21, 2015

Rob--W commented Aug 22, 2015

Rob--W commented Aug 26, 2015

yurydelendik commented Aug 27, 2015

pdfjsbot commented Aug 27, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 27, 2015

From: Bot.io (Windows)

Success

Published

yurydelendik commented Aug 27, 2015