Viewer: enable find functionality for small devices #8132

timvandermeij · 2017-03-04T22:25:16Z

The commit messages provide more information about the intent of each patch.

This PR is a re-do of #6274, so please refer to that PR for previous discussion on this subject. This patch aims to resolve the issues mentioned there while keeping the code changes minimal.

Fixes #6191.
Supersedes #6274.

Reviewing is easier with https://github.com/mozilla/pdf.js/pull/8132/files?w=1.

Snuffleupagus · 2017-03-04T22:48:01Z

This looks a lot cleaner than the previous solution, which is really nice!
A couple of quick things that I noticed here, based on an initial look/test, if you don't mind:

One thing I couldn't help notice about this patch though, is that it (in my testing) unfortunately doesn't seem to address the issue described in the screen-shot in Enabling find functionality on small devices #6274 (comment).
The split toolbar button margin has been decreased slightly to prevent the issue described in Enabling find functionality on small devices #6274 where the "Phrase not found" text would be positioned below the "Highlight all" checkbox.

Without having tested this in detail, to me this sounds a bit too "magical" on its own and I'd worry that it's perhaps not guaranteed to work well on all platforms (since e.g. checkboxes can have different sizes).
Hence I would really recommend enforcing a height: 32px; for the new findbar "container" div elements to try and prevent any issues here!

timvandermeij · 2017-03-04T23:03:05Z

One thing I couldn't help notice about this patch though, is that it (in my testing) unfortunately doesn't seem to address the issue described in the screen-shot in #6274 (comment).

I did look into that, but I really think there is nothing we can do about that. It looks like it's because the find dialog has an absolute position, in which case the browser attempts to use the full width unless a fixed width is specified, which we don't want for the reponsiveness. As you said in #6274 (comment), I also think we need to live with that, and I personally don't find it too much of an issue because usually the space is used reasonably well by all elements.

Without having tested this in detail, to me this sounds a bit too "magical" on its own and I'd worry that it's perhaps not guaranteed to work well on all platforms (since e.g. checkboxes can have different sizes).

You're right. I reverted that changed and enforced a 32 pixel height for all findbar container divs instead, which solves the issue as well and ensures that we don't have to touch the existing split toolbar buttons.

timvandermeij · 2017-03-04T23:04:45Z

/botio-linux preview

Snuffleupagus · 2017-03-04T23:24:26Z

I have looked into that, but I really think there is nothing we can do about that.

Well, if we just use a little bit of JavaScript that runs only when the findbar is opened (and when the viewer resizes while the findbar is open), then I think that could actually be doable :-)

timvandermeij · 2017-03-04T23:26:33Z

Do you mean something like https://github.com/mozilla/pdf.js/blob/master/web/secondary_toolbar.js#L225, but then for the find bar width? If so, I think that should be fine. I do wonder what width we would set in this case, because the width has to be determined automatically based on the contents. Do you have an idea how to do that?

Snuffleupagus · 2017-03-04T23:45:35Z

A very quick proof-of-concept idea: https://gist.github.com/Snuffleupagus/f8640bb3a742fc8f9bed26c6b7c98e5d.

Snuffleupagus · 2017-03-05T08:54:07Z

web/viewer.html

+            <label for="findHighlightAll" class="toolbarLabel" data-l10n-id="find_highlight">Highlight all</label>
+            <input type="checkbox" id="findMatchCase" class="toolbarField" tabindex="95">
+            <label for="findMatchCase" class="toolbarLabel" data-l10n-id="find_match_case_label">Match case</label>
+            <span id="findResultsCount" class="toolbarLabel hidden"></span>


Perhaps it'd make sense to place this in the findbarMessageContainer instead?
Especially since I seem to recall a WIP PR that wanted to add a checkbox for toggling "phrase/word" search, in which case the resultsCount might not fit here any more.

I think you're right. I'll change that.

Snuffleupagus · 2017-03-05T08:56:59Z

web/viewer.html

-            </button>
+        <div class="findbar hidden doorHanger" id="findbar">
+          <div id="findbarLabelContainer">
+            <label for="findInput" class="toolbarLabel" data-l10n-id="find_label">Find:</label>


I'm not sure that I understand why the label is in it's own container here, would you mind explaining why it's not just placed in the findInputContainer instead?

Ninja edit: Oh, I suppose that you may be worried about the different lengths of the label depending on the locale and that it might not fit inside of the min-width otherwise?

One way to solve that, inspired by e.g. the Firefox built-in findbar, and the fact that we don't use labels in the main UI (i.e. not for e.g. the pageNumber nor the zoomDropdown), could be to replace the label with a placeholder in the input instead.

Looking through my local patches/diffs, I actually found this https://gist.github.com/Snuffleupagus/cbba3a6823c3a02a59a1a1d8bcbe6059. As far as I can tell I never submitted a PR for that, but if memory serves it worked just fine.

Yes, I put that in its own label because of the length of the "Search" label in different locales. I actually really like the placeholder idea, so I'll check if that works.

timvandermeij · 2017-03-06T23:05:48Z

I have included your two patches to convert the label to a placeholder/tootip and to fix the problem of find bar width being too large. Moreover, I added a bit of CSS to prevent issues with long find messages on small screens (especially for the Dutch locale with its long strings). The only thing I couldn't do is move the find count to the find message container as that gives some strange wrapping when the screen becomes smaller (at least for the Dutch locale). This may be addressed in a follow-up patch though for when additional checkboxes are added.

timvandermeij · 2017-03-06T23:06:24Z

/botio-linux preview

timvandermeij · 2017-03-06T23:29:40Z

/botio-linux preview

Snuffleupagus · 2017-03-06T23:48:20Z

I've done some quick testing in Firefox, Chrome and IE (on Windows) with the latest version of the PR, and everything seems to work as it should. I also tested RTL a bit, and couldn't find any issues there either.
So this is looking really nice, and a very good improvement overall :-)

The only other thing that we might want to consider (as mentioned on IRC), now that the variability of the "Find" label is gone, is to maybe increase the width of the Find input a bit!?
At least in Firefox it looks too narrow compared to the rest of the findbar, and I'm not sure if the placeholders (in some locales) will realistically fit given the current width of the input.

Another question (adding it here since using inline comments just marked it as "outdated"):
Is it too much of a hassle to move the "placeholder" commit such that it's the first one in the series, since then you'd not need to first add the findbarLabelContainer and the immediately remove it in the next commit thus reducing the churn slightly in this code?

Snuffleupagus · 2017-03-06T23:59:53Z

web/pdf_find_bar.js

+      var inputContainerHeight = this.bar.firstElementChild.clientHeight;
+
+      if (findbarHeight > inputContainerHeight) {
+        // The findbar is higher than the input container, which means that


Nit: I think you want to use taller (instead of higher).

Snuffleupagus · 2017-03-07T00:10:14Z

web/pdf_find_bar.js

+
+      // The find bar has an absolute position and thus the browser extends
+      // its width to the maximum possible width once the find bar does not fit
+      // entirely within the window anymore (and needs to be wrapped). Here we


Nit: Maybe ... (and parts/all of it is automatically wrapped)... or something instead, since the wrapping is automatic based on the CSS.

timvandermeij · 2017-03-08T21:59:48Z

/botio-linux preview

This is common in the rest of the UI and helps us prevent responsiveness issues for different length strings in different locales.

The find functionality is currently not available for small devices because the find dialog is not responsive. This patch fixes that. To achieve this goal, the HTML is changed to always show the find button. To prevent issues because of the addition of an extra button for small views, the previous/next page buttons are hidden if the view becomes small. These buttons are not useful anyway because on small devices navigation is usually done via scrolling. The find functionality is much more useful to have in this case. Moreover, we wrap the existing elements into separate `div`s so that the browser can position the elements itself when the view becomes smaller and logically connected elements stay together when this happens. In the CSS, extra rules for the find bar have been added to ensure that the dialog's doorhanger is always below the find button. All findbar `div`s are forced to be 32 pixels high to prevent the find message text being aligned under the checkboxes. Finally, the find message is only visible when there is actually text to display. This prevents wrapping issues because, by default, the label has padding and margin even if there is no text.

This patch ensures that the find bar is not extended to the window width when element wrapping occurs on small screens.

timvandermeij · 2017-03-08T23:00:41Z

/botio-linux preview

pdfjsbot · 2017-03-08T23:00: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/00208934cb71170/output.txt

pdfjsbot · 2017-03-08T23:02:52Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/00208934cb71170/output.txt

Total script time: 2.16 mins

Published

Snuffleupagus

(This should have been type "approved" instead of "comment", but it doesn't seem that can be changed.)

Typo

Snuffleupagus

This looks much better than the current unresponsive findbar; thanks for seeing this work through!

I obviously cannot review the parts I helped write, but the rest looks good to me :-)
Should we just land this, or do we want to wait until e.g. tomorrow to have a bit more time to play with the PR before merging?

…ness Viewer: enable find functionality for small devices

timvandermeij added the viewer label Mar 4, 2017

timvandermeij force-pushed the findbar-responsiveness branch from 23e13b6 to 0e72e5e Compare March 4, 2017 22:33

timvandermeij force-pushed the findbar-responsiveness branch from 0e72e5e to 8227a18 Compare March 4, 2017 22:55

Snuffleupagus reviewed Mar 5, 2017

View reviewed changes

timvandermeij force-pushed the findbar-responsiveness branch from 8227a18 to e46eb8e Compare March 6, 2017 23:02

timvandermeij force-pushed the findbar-responsiveness branch from e46eb8e to c218958 Compare March 6, 2017 23:28

Snuffleupagus reviewed Mar 6, 2017

View reviewed changes

Snuffleupagus reviewed Mar 7, 2017

View reviewed changes

timvandermeij force-pushed the findbar-responsiveness branch from c218958 to aea9cf4 Compare March 8, 2017 21:54

Snuffleupagus and others added 3 commits March 8, 2017 23:54

Viewer: replace find label with placeholder/tooltip

c79e5b3

This is common in the rest of the UI and helps us prevent responsiveness issues for different length strings in different locales.

Viewer: consistently wrap find bar elements for small screen sizes

b151666

This patch ensures that the find bar is not extended to the window width when element wrapping occurs on small screens.

timvandermeij force-pushed the findbar-responsiveness branch from aea9cf4 to b151666 Compare March 8, 2017 22:58

Snuffleupagus reviewed Mar 8, 2017

View reviewed changes

Snuffleupagus previously approved these changes Mar 8, 2017

View reviewed changes

Snuffleupagus approved these changes Mar 8, 2017

View reviewed changes

timvandermeij merged commit 6908f14 into mozilla:master Mar 9, 2017

timvandermeij deleted the findbar-responsiveness branch March 9, 2017 22:03

Snuffleupagus mentioned this pull request Jul 5, 2017

Ensure that PDFFindBar._adjustWidth is called in all situations where the width of the findbar might have changed #8609

Merged

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

Merge pull request mozilla#8132 from timvandermeij/findbar-responsive…

95c976f

…ness Viewer: enable find functionality for small devices

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Viewer: enable find functionality for small devices #8132

Viewer: enable find functionality for small devices #8132

timvandermeij commented Mar 4, 2017 •

edited

Loading

Snuffleupagus commented Mar 4, 2017 •

edited

Loading

timvandermeij commented Mar 4, 2017 •

edited

Loading

timvandermeij commented Mar 4, 2017

Snuffleupagus commented Mar 4, 2017

timvandermeij commented Mar 4, 2017 •

edited

Loading

Snuffleupagus commented Mar 4, 2017

Snuffleupagus Mar 5, 2017 •

edited

Loading

timvandermeij Mar 5, 2017

Snuffleupagus Mar 5, 2017 •

edited

Loading

Snuffleupagus Mar 5, 2017 •

edited

Loading

timvandermeij Mar 5, 2017

timvandermeij commented Mar 6, 2017

timvandermeij commented Mar 6, 2017

timvandermeij commented Mar 6, 2017

Snuffleupagus commented Mar 6, 2017 •

edited

Loading

Snuffleupagus Mar 6, 2017

Snuffleupagus Mar 7, 2017

timvandermeij commented Mar 8, 2017

timvandermeij commented Mar 8, 2017

pdfjsbot commented Mar 8, 2017

pdfjsbot commented Mar 8, 2017

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus left a comment

Viewer: enable find functionality for small devices #8132

Viewer: enable find functionality for small devices #8132

Conversation

timvandermeij commented Mar 4, 2017 • edited Loading

Snuffleupagus commented Mar 4, 2017 • edited Loading

timvandermeij commented Mar 4, 2017 • edited Loading

timvandermeij commented Mar 4, 2017

Snuffleupagus commented Mar 4, 2017

timvandermeij commented Mar 4, 2017 • edited Loading

Snuffleupagus commented Mar 4, 2017

Snuffleupagus Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

timvandermeij Mar 5, 2017

Choose a reason for hiding this comment

Snuffleupagus Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

timvandermeij Mar 5, 2017

Choose a reason for hiding this comment

timvandermeij commented Mar 6, 2017

timvandermeij commented Mar 6, 2017

timvandermeij commented Mar 6, 2017

Snuffleupagus commented Mar 6, 2017 • edited Loading

Snuffleupagus Mar 6, 2017

Choose a reason for hiding this comment

Snuffleupagus Mar 7, 2017

Choose a reason for hiding this comment

timvandermeij commented Mar 8, 2017

timvandermeij commented Mar 8, 2017

pdfjsbot commented Mar 8, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Mar 8, 2017

From: Bot.io (Linux)

Success

Published

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

timvandermeij commented Mar 4, 2017 •

edited

Loading

Snuffleupagus commented Mar 4, 2017 •

edited

Loading

timvandermeij commented Mar 4, 2017 •

edited

Loading

timvandermeij commented Mar 4, 2017 •

edited

Loading

Snuffleupagus Mar 5, 2017 •

edited

Loading

Snuffleupagus Mar 5, 2017 •

edited

Loading

Snuffleupagus Mar 5, 2017 •

edited

Loading

Snuffleupagus commented Mar 6, 2017 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading