Explain double-click behaviour on tool tip #7717

jeenuv · 2016-10-13T09:11:58Z

The outline toggle button has a feature where it can be double-clicked
to expand/collapse all items shown therein. Although this is described
in the FAQ, can go potentially unnoticed. This, however, being a useful
feature, advertise on the tool tip itself.

l10n translations are untouched.

Signed-off-by: Jeenu Viswambharan [email protected]

timvandermeij · 2016-10-13T11:49:20Z

The localization logic will override this, so this change is incomplete. Instead, you need to edit https://github.com/mozilla/pdf.js/blob/master/l10n/en-US/viewer.properties#L102 and change that to Show Document Outline (double-click to expand/collapse all) and use this same string at 8978064#diff-a72ef320c3225c98b2163b89a3e8ff62R84 as well.

Fixes #7713.

timvandermeij · 2016-10-13T11:53:29Z

We might need to change the L10n ID (from outline to document_outline for example) to make the localization teams aware of the change, but I'm not sure if that is actally required.

/cc @yurydelendik @Snuffleupagus What do you think?

jeenuv · 2016-10-13T12:02:48Z

@timvandermeij That's what I did on a local version of this patch, but then I read in contribution guide:

Note that the translations for PDF.js in the l10n folder are synchronized with the Aurora branch of Mozilla Firefox. This means that we will only accept pull requests that add strings currently missing in the Aurora branch (as it will take at least six weeks before the most recent translations are in the Aurora branch), but keep in mind that the changes will be overwritten when we synchronize again.

My reading was that the patch won't be accepted as it'd be overwritten anyway. I'm happy to push that version instead. What do you think?

Snuffleupagus · 2016-10-13T12:06:39Z

We might need to change the L10n ID to make the localization teams aware of the change, but I'm not sure if that is actually required.

As explained in https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings, you _must always_ update the l10n id when changing strings, since otherwise the tools used by localizers may not pick up the change!

[...] and change that to Show Document Outline (double-click to expand/collapse all)

Provided that we actually want to change the tooltip unconditionally like this PR suggest, I'm not really sure about the wording used here.
It seems to me that there's something missing after all, perhaps it should be [...] all items), since it doesn't feel entirely clear what's actually being expanded/collapsed otherwise (e.g. is it the outline itself?).

timvandermeij · 2016-10-13T12:09:04Z

I agree with the [...] all items change.

@jeenuv Everything except for the en-US file will be synchronized. We only make changes in the English file and upstream takes care of the other translations that we periodically import.

jeenuv · 2016-10-13T18:52:09Z

I've updated the PR. Please check.

timvandermeij · 2016-10-13T19:45:56Z

This looks better, but we need to change outline.title and outline_label to document_outline.title and document_outline_label, respectively. This ID must change to make sure that the localization teams know that they have to update the translation of this string. You need to change this in the en-US locale file and in viewer.html (the data-l10n-id locations).

timvandermeij · 2016-10-14T14:09:08Z

l10n/en-US/viewer.properties

@@ -99,8 +99,8 @@ print_progress_close=Cancel
 # tooltips)
 toggle_sidebar.title=Toggle Sidebar
 toggle_sidebar_label=Toggle Sidebar
-outline.title=Show Document Outline
-outline_label=Document Outline
+document_outline_label=Show Document Outline (double-click to expand/collapse all items)


The label and title IDs must be swapped, i.e., this should be the title, just like before.

timvandermeij · 2016-10-14T14:09:36Z

web/viewer.html

@@ -81,8 +81,8 @@
            <button id="viewThumbnail" class="toolbarButton group toggled" title="Show Thumbnails" tabindex="2" data-l10n-id="thumbs">
               <span data-l10n-id="thumbs_label">Thumbnails</span>
            </button>
-            <button id="viewOutline" class="toolbarButton group" title="Show Document Outline" tabindex="3" data-l10n-id="outline">
-               <span data-l10n-id="outline_label">Document Outline</span>
+            <button id="viewOutline" class="toolbarButton group" title="Show Document Outline (double-click to expand/collapse all items)" tabindex="3" data-l10n-id="outline">


The data-l10n-id in this line should also be document_outline.

timvandermeij · 2016-10-14T14:10:21Z

web/viewer.html

-            <button id="viewOutline" class="toolbarButton group" title="Show Document Outline" tabindex="3" data-l10n-id="outline">
-               <span data-l10n-id="outline_label">Document Outline</span>
+            <button id="viewOutline" class="toolbarButton group" title="Show Document Outline (double-click to expand/collapse all items)" tabindex="3" data-l10n-id="outline">
+               <span data-l10n-id="document_outline.title">Document Outline</span>


The label ID must be used here, just like before, so this should be document_outline_label.

timvandermeij

Looks good to me after this last change.

timvandermeij · 2016-10-15T18:14:27Z

web/viewer.html

-            <button id="viewOutline" class="toolbarButton group" title="Show Document Outline" tabindex="3" data-l10n-id="outline">
-               <span data-l10n-id="outline_label">Document Outline</span>
+            <button id="viewOutline" class="toolbarButton group" title="Show Document Outline (double-click to expand/collapse all items)" tabindex="3" data-l10n-id="document_outline">
+                <span data-l10n-id="document_outline_label">Outline</span>


Revert the changes on this line except for the data-l10n-id change. The extra space is not necessary as we indent everything with two spaces and the text should remain the same as in the L10n file.

Ah, sorry - that must be my editor!

The word Document has disappeared in this line, so that needs to be put back. Therefore, this line should be:

<span data-l10n-id="document_outline_label">Document Outline</span>

The outline toggle button has a feature where it can be double-clicked to expand/collapse all items shown therein. Although this is described in the FAQ, can go potentially unnoticed. This, however, being a useful feature, advertise on the tool tip itself. l10n translation for en-US and IDs updated. Signed-off-by: Jeenu Viswambharan <[email protected]>

jeenuv · 2016-10-16T08:59:26Z

Boy, can't I make a proper commit?! Hopefully I get this one right. Thanks for your patience.

timvandermeij · 2016-10-16T12:09:41Z

/botio-linux preview

pdfjsbot · 2016-10-16T12:09:42Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2016-10-16T12:11:36Z

From: Bot.io (Linux)

Success

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

Total script time: 1.91 mins

Published

timvandermeij

Looks good to me. Since this is a change that affects upstream work for translations, could you merge this if you like it as well, @yurydelendik?

timvandermeij · 2016-10-18T11:52:40Z

Thank you!

Explain double-click behaviour on tool tip

timvandermeij added the feature label Oct 13, 2016

jeenuv force-pushed the display-double-click-tooltip branch 2 times, most recently from a010c53 to 6e51163 Compare October 13, 2016 18:51

jeenuv force-pushed the display-double-click-tooltip branch from 6e51163 to 1b53510 Compare October 13, 2016 20:22

timvandermeij reviewed Oct 14, 2016

View reviewed changes

jeenuv force-pushed the display-double-click-tooltip branch from 1b53510 to 088a071 Compare October 15, 2016 14:42

timvandermeij reviewed Oct 15, 2016

View reviewed changes

jeenuv force-pushed the display-double-click-tooltip branch from 088a071 to c65175c Compare October 15, 2016 18:33

jeenuv force-pushed the display-double-click-tooltip branch from c65175c to f2dcacd Compare October 16, 2016 08:59

timvandermeij approved these changes Oct 16, 2016

View reviewed changes

timvandermeij assigned yurydelendik Oct 16, 2016

yurydelendik approved these changes Oct 18, 2016

View reviewed changes

timvandermeij merged commit 6906623 into mozilla:master Oct 18, 2016

timvandermeij mentioned this pull request Oct 18, 2016

Chrome extension: Add option/Default to collapse all items in outline #7713

Closed

Snuffleupagus mentioned this pull request Jan 19, 2017

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

Merged

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

Merge pull request mozilla#7717 from jeenuv/display-double-click-tooltip

1cffe56

Explain double-click behaviour on tool tip

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explain double-click behaviour on tool tip #7717

Explain double-click behaviour on tool tip #7717

jeenuv commented Oct 13, 2016

timvandermeij commented Oct 13, 2016 •

edited

Loading

timvandermeij commented Oct 13, 2016 •

edited

Loading

jeenuv commented Oct 13, 2016

Snuffleupagus commented Oct 13, 2016 •

edited

Loading

timvandermeij commented Oct 13, 2016

jeenuv commented Oct 13, 2016

timvandermeij commented Oct 13, 2016 •

edited

Loading

timvandermeij Oct 14, 2016

timvandermeij Oct 14, 2016

timvandermeij Oct 14, 2016

timvandermeij left a comment

timvandermeij Oct 15, 2016

jeenuv Oct 15, 2016

timvandermeij Oct 15, 2016

jeenuv commented Oct 16, 2016

timvandermeij commented Oct 16, 2016

pdfjsbot commented Oct 16, 2016

pdfjsbot commented Oct 16, 2016

timvandermeij left a comment

timvandermeij commented Oct 18, 2016

Explain double-click behaviour on tool tip #7717

Explain double-click behaviour on tool tip #7717

Conversation

jeenuv commented Oct 13, 2016

timvandermeij commented Oct 13, 2016 • edited Loading

timvandermeij commented Oct 13, 2016 • edited Loading

jeenuv commented Oct 13, 2016

Snuffleupagus commented Oct 13, 2016 • edited Loading

timvandermeij commented Oct 13, 2016

jeenuv commented Oct 13, 2016

timvandermeij commented Oct 13, 2016 • edited Loading

timvandermeij Oct 14, 2016

Choose a reason for hiding this comment

timvandermeij Oct 14, 2016

Choose a reason for hiding this comment

timvandermeij Oct 14, 2016

Choose a reason for hiding this comment

timvandermeij left a comment

Choose a reason for hiding this comment

timvandermeij Oct 15, 2016

Choose a reason for hiding this comment

jeenuv Oct 15, 2016

Choose a reason for hiding this comment

timvandermeij Oct 15, 2016

Choose a reason for hiding this comment

jeenuv commented Oct 16, 2016

timvandermeij commented Oct 16, 2016

pdfjsbot commented Oct 16, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Oct 16, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij left a comment

Choose a reason for hiding this comment

timvandermeij commented Oct 18, 2016

timvandermeij commented Oct 13, 2016 •

edited

Loading

timvandermeij commented Oct 13, 2016 •

edited

Loading

Snuffleupagus commented Oct 13, 2016 •

edited

Loading

timvandermeij commented Oct 13, 2016 •

edited

Loading