Secondary toolbar #2792

timvandermeij · 2013-02-24T16:52:41Z

Implements the secondary toolbar as discussed in #2714.

Snuffleupagus · 2013-02-24T17:17:45Z

@timvandermeij First of all, nice that you're working on this!
A few very quick observations:

The name settingsContainer seems wrong to me, why not secondaryToolbar or similar. Since this is really just another toolbar, why not including that word in the name. (What you had in your first mock-up in Add new UI elements #2714 is a settings UI in my opinion.)
Also, when you do comparisons remember to use strict equalities ===/!==.

Edit:

Only slide the viewerContainer, not the mainContainer (couldn't get this to work properly, because if I did this, the bar would move into the viewerContainer, but would not move the viewerContainer itself).

I would suggest that this new toolbar should overlay the document (perhaps with a slight opaque background colour),
similar to how the sidebar works at lower resolutions. To see this behaviour in action, try "Responsive Design View" from the "Web Developer" menu in Firefox (Ctrl+Shift+M).

waddlesplash · 2013-02-25T18:16:03Z

/botio-windows preview

pdfjsbot · 2013-02-25T18:16:05Z

From: Bot.io (Windows)

Received

Command cmd_preview from @waddlesplash received. Current queue size: 1

Live output at: http://107.22.172.223:8877/945e81b72442ac0/output.txt

pdfjsbot · 2013-02-25T18:23:02Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/945e81b72442ac0/output.txt

Total script time: 0.14 mins

waddlesplash · 2013-02-25T18:26:15Z

@timvandermeij there are apparently some conflicts with the current master, can you fix?

timvandermeij · 2013-02-25T19:50:36Z

@Snuffleupagus: I will address your comments as soon as possible. I'm currently very busy, but I expect that I can address your comments and do some additional finetuning to the settings bar in the next two days.

@waddlesplash: I'll fix this also in the next two days.

waddlesplash · 2013-02-25T19:53:20Z

You need to pull and fix conflicts, apparently.

timvandermeij · 2013-02-25T19:56:10Z

Oh, I see. I'll also do that in the next few days.

timvandermeij · 2013-02-26T23:05:43Z

I have improved my design. Changes are:

The hover styles are now the same as the hover styles of the outline bar.
The text color is consistent.
All options that are currently available are added to the settings bar (I have also designed icons for all of them).
I have introduced categories to categorize all possible options.
I have added a tick to indicate a selected mode.

Please take a look at the render of the design below and let me know what you think about this improved design. If you like the changes, I'll implement them when I upload my new code for the settings bar.

Note: I'm not a professional designer, so more experienced UX designers can probably make some more improvements. :)

@jviereck: the PSD file of this new design can be downloaded from here: [removed, old design]

The design render is below:

waddlesplash · 2013-02-26T23:12:03Z

The new design looks much better than the old one!

timvandermeij · 2013-02-26T23:13:18Z

@waddlesplash: thank you! Really nice to know that you like it! I will fix the conflicts and the strict equalities issue that @Snuffleupagus reported right now. Then, I'll push the changes such that a new preview can be made. Tomorrow, I'll fix the other issues that @Snuffleupagus reported and start on implementing the new design.

timvandermeij · 2013-02-26T23:30:43Z

Conflicts are now resolved and strict equality issue is also fixed. I think a new preview can be generated now. I guess there might be one problem: I used 'git pull mozilla master' to get all new changes and to be able to fix the conflicts, but in my commit above I see that also all of your changes are pushed. Is that a problem (maybe if I squash everything later, it won't be a big deal) and if yes, how can I fix it such that only my changes get pushed?

Snuffleupagus · 2013-02-26T23:32:46Z

/botio-linux preview

pdfjsbot · 2013-02-26T23:32:56Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/4191b9bf9789ae2/output.txt

pdfjsbot · 2013-02-26T23:33:06Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/4191b9bf9789ae2/output.txt

Total script time: 0.16 mins

Published

Viewer: http://107.21.233.14:8877/4191b9bf9789ae2/web/viewer.html
Extension: http://107.21.233.14:8877/4191b9bf9789ae2/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/4191b9bf9789ae2/extensions/firefox/pdf.js.amo.xpi

Snuffleupagus · 2013-02-26T23:51:47Z

Looks nice, you've made quite a bit of progress!
Some very quick feedback:

I don't think that this toolbar should close automatically. It should rather be done either by clicking the button again or by pressing the Escape key. The current behaviour becomes quite tedious when you want to change a number of different options in a row.
Again, I personally don't like this secondary toolbar changing the size of the viewport. In my opinion it should overlay the viewport instead.

@timvandermeij As you suggested, just squash the commits and it will be OK.

Also, I really think that we need feedback on this from UI/UX sooner rather than later, to prevent having to change to many things later on.

waddlesplash · 2013-02-27T00:38:32Z

This doesn't work on things with small screen size, e.g. mobile devices. Example: 7" tablet: See screenshot. (This will be fixed by addressing @Snuffleupagus's comment 2.) How about replicating what the viewer does when it has a small screen and needs to display the sidebar, except ALWAYS do it that way. (Hint: try it in Responsive Design View with a screen width of around 455px).

As for the patch, ignore it. It appears fine in the "files changed" so it should be fine elsewhere.

timvandermeij · 2013-02-27T09:40:38Z

I agree. The viewport size should not change. It is item 1 on the to-do list, but thanks tot your comments I have some more ideas to fix it. Will look into it later this day.

waddlesplash · 2013-02-27T17:54:23Z

/botio-linux preview

pdfjsbot · 2013-02-27T17:54:27Z

From: Bot.io (Linux)

Received

Command cmd_preview from @waddlesplash received. Current queue size: 2

Live output at: http://107.21.233.14:8877/8a748032947f353/output.txt

pdfjsbot · 2013-02-27T18:02:48Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/8a748032947f353/output.txt

Total script time: 0.28 mins

Published

Viewer: http://107.21.233.14:8877/8a748032947f353/web/viewer.html
Extension: http://107.21.233.14:8877/8a748032947f353/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/8a748032947f353/extensions/firefox/pdf.js.amo.xpi

timvandermeij · 2013-02-27T18:06:03Z

I added a new commit. Can somebody generate a new preview?

waddlesplash · 2013-02-27T18:11:26Z

/botio-linux preview

pdfjsbot · 2013-02-27T18:11:31Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/4198d67c1492112/output.txt

pdfjsbot · 2013-02-27T18:11:48Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/4198d67c1492112/output.txt

Total script time: 0.23 mins

Published

Viewer: http://107.21.233.14:8877/4198d67c1492112/web/viewer.html
Extension: http://107.21.233.14:8877/4198d67c1492112/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/4198d67c1492112/extensions/firefox/pdf.js.amo.xpi

timvandermeij · 2013-02-27T18:12:25Z

Nice, thanks! Now I can test it on my Nexus 4 phone :-D

waddlesplash · 2013-02-27T18:14:01Z

Does not work for me, it overlaps the scrollbar:

timvandermeij · 2013-02-27T18:15:27Z

Yes, I added that bug to the to-do list above. I'm not sure how to move the scrollbar. Any ideas?

EDIT: with the new commit, it also looks much better on my Nexus 4 phone. :)

gigaherz · 2013-02-27T19:25:38Z

I think you shouldn't move the scrollbar, you should account for it instead. Maybe with some javascript magic that calculates the effective width of the toolbar - the width of the content area, and sets the result as margin-right of the panel?

brendandahl · 2013-08-12T17:54:23Z

/botio-linux preview

pdfjsbot · 2013-08-12T17:54:24Z

From: Bot.io (Linux)

Received

Command cmd_preview from @brendandahl received. Current queue size: 2

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

pdfjsbot · 2013-08-12T18:12:41Z

From: Bot.io (Linux)

Success

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

Total script time: 0.26 mins

Published

Viewer: http://107.21.233.14:8877/a5c7393622771b2/web/viewer.html
Extension: http://107.21.233.14:8877/a5c7393622771b2/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/a5c7393622771b2/extensions/firefox/pdf.js.amo.xpi

brendandahl · 2013-08-12T20:01:01Z

web/secondary_toolbar.js

+
+'use strict';
+
+var secondaryToolbar = {


Uppercase S

Fixed in new commit.

brendandahl · 2013-08-12T20:05:18Z

Looking mostly good but a few more things:

The right click context menu seems to be broken now.
Somewhat of an edge case but If you make window below the min width of 320 the secondary toolbar still moves with the window, I'd expect it to stay lined up with the button.

timvandermeij · 2013-08-13T17:28:22Z

@brendandahl The nits are fixed. Note that the context menu is not broken, but just disabled in the regular mode. This has been done on purpose since all the functionality has been moved to the secondary toolbar. The context menu wasn't considered a good place for that kind of functionality. Important to note is that the context menu is available in presentation mode, since you don't have the secondary toolbar there. Does that answer your question or should we change anything about that?

@Snuffleupagus Do you have an idea on how we can fix @brendandahl's second point? I do not know a solution right now, but I'll also think about it.

Snuffleupagus · 2013-08-13T18:20:27Z

@Snuffleupagus Do you have an idea on how we can fix @brendandahl's second point? I do not know a solution right now, but I'll also think about it.

Yes, looking at the code I think I know why it fails. I'll fix this, and also address the lint failures while I'm at it.
Btw, I noticed that a CSS class seems to have gone missing in a HTML tag, probably as a result of rebasing this PR.

Snuffleupagus · 2013-08-13T18:45:17Z

web/viewer.html

+              <span data-l10n-id="page_rotate_ccw_label">Rotate Counterclockwise</span>
+            </button>
+          </div>
+	</div>  <!-- secondaryToolbar -->


I apparently forgot to indent this line properly.

@timvandermeij Can you fix this when you squash?

Snuffleupagus · 2013-08-13T19:03:19Z

Hopefully the final preview!

/botio-linux preview

pdfjsbot · 2013-08-13T19:03:21Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/4a133c5e01c2223/output.txt

pdfjsbot · 2013-08-13T19:03:33Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/4a133c5e01c2223/output.txt

Total script time: 0.20 mins

Published

Viewer: http://107.21.233.14:8877/4a133c5e01c2223/web/viewer.html
Extension: http://107.21.233.14:8877/4a133c5e01c2223/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/4a133c5e01c2223/extensions/firefox/pdf.js.amo.xpi

timvandermeij · 2013-08-17T15:12:10Z

Rebased this PR to the tip to avoid any problems when merging. Note: no code changes were made!

Snuffleupagus · 2013-08-31T20:08:18Z

l10n/en-US/viewer.properties

 presentation_mode.title=Switch to Presentation Mode
 presentation_mode_label=Presentation Mode
-open_file.title=Open File
-open_file_label=Open
+open_file.title=Open a file for viewing


I just noticed this, and I'm a little curious as to why this was changed!?

@Snuffleupagus I have absolutely no idea... Maybe an old test or something? Although I think the change is not that bad, should we revert it?

The reason that I ask is that if you keep this change, you need to upper-case parts of it so that it follows the language style of the other strings. (Something like Open a File for Viewing, but please note that I'm not a native English speaker.)

Personally I think that this change is unnecessary, since "Open File" is a standard expression in software (and it won't even be visible in Firefox since that button is hidden there).
Since I guess that you need to rebase and squash anyway, it shouldn't be to hard to change back. :-)

You're right. It is fixed now. I've also solved merge conflicts and squashed everything again.

timvandermeij · 2013-09-01T20:48:31Z

Rebased this PR to the tip. Only two language strings in the Dutch and English language files were changed. The rest of the code is left untouched.

shorlander · 2013-09-04T15:41:16Z

Making the panel darker does not seem consistent with the findbar.

I would suggest making any overlay panels darker, but probably out of scope for this :)

By the lighter box, we assume he means the intended hover effect, so that is okay.

There is some kind of weird ghosting going on when hovering buttons

Seems to be some kind of strange interaction with the gradient on <div id="secondaryToolbar"… But I don't know why it would do that.

Snuffleupagus · 2013-09-04T16:01:33Z

@shorlander That is really strange, I'm not able to reproduce this on Windows. This is what it looks like on my computer:

If you open the findbar, are these issues present there as well? Since the secondary toolbar uses mostly already existent CSS, the issues you are seeing seem really weird.
I also noticed that the text on the buttons looks a bit blurry in your screenshot, which I'm not able to reproduce either.

shorlander · 2013-09-04T16:06:17Z

@Snuffleupagus Yeah it's kind of weird, only happens on OSX. Looking closer it doesn't look like the gradient is being drawn on the whole panel just buttons on hover. Findbar does not have this issue.

Snuffleupagus · 2013-09-04T17:07:30Z

Looking closer it doesn't look like the gradient is being drawn on the whole panel just buttons on hover.

Looking at your screenshot, I agree with this observation.
I wonder if the issues are somehow related to CSS specificity, but I would assume that it is implemented the same way in the browser independent of what operating system it runs on. Maybe I'm wrong?

@shorlander Just to clarify, when you say it happens on OSX, you mean Firefox on OSX right?

timvandermeij · 2013-09-04T19:55:41Z

New toggle icon from @shorlander has been added and the PR has been rebased to the tip and squashed.

Now we only need to fix that strange hover effect and then this should be ready to go. Panels look fine to me for now (and changing all panels is out of scope for this PR), but they can certainly be made darker in a follow-up PR.

Snuffleupagus · 2013-09-04T19:57:01Z

@timvandermeij Sorry about being afk on IRC! If you want to talk, I'm there now.

shorlander · 2013-09-04T20:02:42Z

If it seems to be a Mac Firefox specific bug I can just file a bug against that.

pdfjsbot · 2013-09-04T21:38:20Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/9a9f0471e223291/output.txt

pdfjsbot · 2013-09-04T21:38:40Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/9a9f0471e223291/output.txt

Total script time: 0.32 mins

Published

Viewer: http://107.22.172.223:8877/9a9f0471e223291/web/viewer.html
Extension: http://107.22.172.223:8877/9a9f0471e223291/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.22.172.223:8877/9a9f0471e223291/extensions/firefox/pdf.js.amo.xpi

timvandermeij · 2013-09-04T21:54:32Z

@shorlander After a few hours of debugging this PR with Yury and Brendan, it turns out to be a known bug: https://bugzilla.mozilla.org/show_bug.cgi?id=902591 (and https://bugzilla.mozilla.org/show_bug.cgi?id=905246). Even Yury and Brendan, both OS X users, couldn't both replicate it (Brendan could, Yury couldn't, using the same configuration), so it's very intermittent. Since it is not really a problem of this particular PR but more of Firefox itself, and it is known upstream, this PR should be ready to go now.

Still I'd like to thank you for helping with the reviewing process!

Secondary toolbar

timvandermeij mentioned this pull request Feb 26, 2013

Add new UI elements #2714

Closed

brendandahl reviewed Aug 12, 2013
View reviewed changes

Snuffleupagus reviewed Aug 13, 2013
View reviewed changes

Snuffleupagus reviewed Aug 31, 2013
View reviewed changes

Snuffleupagus mentioned this pull request Sep 2, 2013

Zoom level detail is not hidden when PDF.js is in small iframe #3140

Closed

Secondary toolbar

3c0ac37

brendandahl added a commit that referenced this pull request Sep 4, 2013

Merge pull request #2792 from timvandermeij/settings-bar

c5bcd7a

Secondary toolbar

brendandahl merged commit c5bcd7a into mozilla:master Sep 4, 2013

timvandermeij deleted the settings-bar branch September 5, 2013 08:54


		'use strict';

		var secondaryToolbar = {

Secondary toolbar #2792

Secondary toolbar #2792

Conversation

timvandermeij commented Feb 24, 2013

Snuffleupagus commented Feb 24, 2013

waddlesplash commented Feb 25, 2013

pdfjsbot commented Feb 25, 2013

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 25, 2013

From: Bot.io (Windows)

Failed

waddlesplash commented Feb 25, 2013

timvandermeij commented Feb 25, 2013

waddlesplash commented Feb 25, 2013

timvandermeij commented Feb 25, 2013

timvandermeij commented Feb 26, 2013

waddlesplash commented Feb 26, 2013

timvandermeij commented Feb 26, 2013

timvandermeij commented Feb 26, 2013

Snuffleupagus commented Feb 26, 2013

pdfjsbot commented Feb 26, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 26, 2013

From: Bot.io (Linux)

Success

Published

Snuffleupagus commented Feb 26, 2013

waddlesplash commented Feb 27, 2013

timvandermeij commented Feb 27, 2013

waddlesplash commented Feb 27, 2013

pdfjsbot commented Feb 27, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 27, 2013

From: Bot.io (Linux)

Success

Published

timvandermeij commented Feb 27, 2013

waddlesplash commented Feb 27, 2013

pdfjsbot commented Feb 27, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 27, 2013

From: Bot.io (Linux)

Success

Published

timvandermeij commented Feb 27, 2013

waddlesplash commented Feb 27, 2013

timvandermeij commented Feb 27, 2013

gigaherz commented Feb 27, 2013

brendandahl commented Aug 12, 2013

pdfjsbot commented Aug 12, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Aug 12, 2013

From: Bot.io (Linux)

Success

Published

brendandahl Aug 12, 2013

Choose a reason for hiding this comment

timvandermeij Aug 13, 2013

Choose a reason for hiding this comment

brendandahl commented Aug 12, 2013

timvandermeij commented Aug 13, 2013

Snuffleupagus commented Aug 13, 2013

Snuffleupagus Aug 13, 2013

Choose a reason for hiding this comment

Snuffleupagus commented Aug 13, 2013

pdfjsbot commented Aug 13, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Aug 13, 2013

From: Bot.io (Linux)

Success

Published

timvandermeij commented Aug 17, 2013

Snuffleupagus Aug 31, 2013

Choose a reason for hiding this comment