Non/less flashing text layer during selection. #6591

yurydelendik · 2015-11-03T19:54:19Z

If we place entire-text-layer div after the content, it will make selection flash in different way: bottom-right part instead of left-top.

-moz-user-select fixes flashing for Firefox. (-webkit-user-select has no effect -- just added it for company).

timvandermeij · 2015-11-03T20:37:53Z

This makes text selection much more smooth in my opinion. Nice work!

Rob--W · 2015-11-03T22:08:10Z

Which steps should I follow to observe the undesiredflashing behavior in Chrome?

I've tried to select text and it is much better than before, but I don't see any other issues (perhaps my steps are wrong? E.g. I select the title and move the mouse downwards, before the change only the first letter is selected, after this commit multiple lines are selected).

yurydelendik · 2015-11-03T22:15:25Z

STR:

Open PDF.js viewer app on default tracemonkey pdf;
Set page 1 at 200% and top-left corner at the top-left corner of the window;
Click and hold in the middle of "Rick Reitmaier#, Michael Bebenita+";
Move mouse slowly towards "University of California, Irvine+"

(Without fix) notice flashing of the text at the top and left.

yurydelendik · 2015-11-03T22:16:51Z

Which steps should I follow to observe the undesiredflashing behavior in Chrome?

We still have flashing of the bottom-right text on other browsers but Firefox.

Rob--W · 2015-11-03T22:58:05Z

Nice find. Is this user-select behavior guaranteed by any standard, or is it an accidental implementation detail? In this specific case, Firefox's result is obviously more desirable, but I cannot really formulate what exactly is wrong with Chrome (and how it can be fixed).

yurydelendik · 2015-11-03T23:04:26Z

Is this user-select behavior guaranteed by any standard,

Based on https://developer.mozilla.org/en-US/docs/Web/CSS/user-select it is not a standard, so it will be hard to find a documented behavior. Looks like FF just ignores any selection action, but other browsers are trying to work around of non-selectable element.

Rob--W · 2015-11-03T23:29:09Z

I found an issue with your patch. If you start with the selection in the blank space between "Just-in-Time" and the authors, then move the mouse down, then:

In Firefox nothing gets selected.
In Chrome all text after the text is selected (until the end of the document).

In both Chrome and Firefox (without patch), the above action results in a text selection starting at the top of the page and ending at the location of the cursor.

So, the behavior for a selection that starts inside text is better, but the result for a selection that starts outside the range is worse.

yurydelendik · 2015-11-04T00:40:43Z

So question: shall I keep patch as is, remove user-select or close entire PR? Voting is opened :)

Rob--W · 2015-11-04T09:59:24Z

Is it an option (= no performance loss and no negative side effects) to insert the element only when the user clicks on a text layer item (via a mousedown event)? That would probably fix Firefox's bug.

Even without the additional fixes, I'm currently in favor of merging the patch because it has better results for the common case (start a selection in the text and move elsewhere).

Snuffleupagus · 2015-11-04T11:29:56Z

To summarize what I wrote on IRC:

It's currently not possible to "cancel" the text-selection by clicking on an empty part of a page. (You need to click on either: a piece of text, or outside of the pages.)
As far as I'm concerned, given the negative UX implications of it, this issue needs to be fixed.
The text-selection cursor is permanently displayed, when the cursor is positioned over a page (regardless if it's over selectable text, or not).
It's no longer possible to start the text-selection just outside of the text. This can make it much more difficult to e.g. select the text contents of a link, unless there is "normal" text surrounding it.
For example, try selecting the text of the vertical link on the first page of http://arxiv.org/pdf/1112.0542v1.pdf, with/without this patch.
This could probably be considered a minor issue, but since I noticed it, I at least wanted to mention it.

yurydelendik · 2015-11-04T14:54:13Z

/botio-windows preview

How about now? Used spin on Rob's idea to insert div only when mouse clicked.

yurydelendik · 2015-11-04T15:36:07Z

Now fixed for useCssZoomOnly

/botio-linux preview

Addresses "the selection gets reversed" concern expressed at #5545 and #4843.

Snuffleupagus · 2015-11-04T16:37:59Z

web/text_layer_builder.js

+      div.addEventListener('mouseup', function (e) {
+        var end = div.querySelector('.endOfContent');
+        if (!end) {
+        }


Missing return; before }?

Fixed. Thanks

Snuffleupagus · 2015-11-04T16:40:14Z

The last version seems to address all of my comments from #6591 (comment), nicely done!

Snuffleupagus · 2015-11-04T18:00:54Z

web/text_layer_builder.css

+
+.textLayer .endOfContent.active {
+  top: 0px;
+  bottom: 0px;


Is this line actually necessary, since it shouldn't change anything given that bottom: 0px; was already set above?

timvandermeij · 2015-11-04T20:53:14Z

/botio-linux preview

pdfjsbot · 2015-11-04T20:53:15Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/5cf13a1f0fe544c/output.txt

pdfjsbot · 2015-11-04T20:54:05Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/5cf13a1f0fe544c/output.txt

Total script time: 0.83 mins

Published

Rob--W · 2015-11-04T21:27:40Z

web/text_layer_builder.js

+     * This reduces flickering of the content if mouse slowly dragged down/up.
+     * @private
+     */
+    _bindMouse: function () {


Rename anonymous function to TextLayerBuilder_bindMouse.

Rob--W · 2015-11-04T22:04:49Z

In Chrome: if you press the mouse at any blank space and move the pointer upwards, then the whole document is selected. Moving by just a few pixels is already enough to trigger this unwanted behavior, so it can already happen if the user just wants to click on the document.

The new _bindMouse logic has made the patch a bit worse. Please disable the body of _bindMouse in every browser except for Firefox (unless you've tested the browser and it showed good results).

yurydelendik · 2015-11-04T22:23:09Z

@Rob--W Most of the logic in _bindMouse for other browsers, especially "set top" part. I cannot reproduce behavior you described above on Chrome for Mac OSX -- in fact the "set top" part suppose to help to remove that.

Rob--W · 2015-11-04T22:30:07Z

@yurydelendik I can trigger the bug at 100% zoom in Chrome 46 (Linux, OS X and Windows).

yurydelendik · 2015-11-04T22:30:41Z

Sorry it's actually fixing the case when selection started from text. Not sure how to address your issue yet -- by removing to above you shall get the same effect but when you are moving down (try http://107.21.233.14:8877/5ce984c56a4c940/web/viewer.html, click empty space and move down)

Rob--W · 2015-11-04T22:35:14Z

@yurydelendik The difference between the previous version and the last version is that in the previous version you had to move over text, whereas in the last version just moving the cursor by a few pixels is already enough.

When I click on the blank space as a user (e.g. to focus the window?), then it is very realistic that the mouse moves by a few pixels. When that happens I don't want the whole document to be selected.

yurydelendik · 2015-11-04T22:52:37Z

@Rob--W okay, suppressed this logic when selection starts at empty space. Is this better?

/botio-windows preview

pdfjsbot · 2015-11-04T22:52:37Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/58bd91f15157c48/output.txt

pdfjsbot · 2015-11-04T22:53:57Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/58bd91f15157c48/output.txt

Total script time: 1.32 mins

Published

Rob--W · 2015-11-04T22:56:22Z

👍 !

Non/less flashing text layer during selection.

Snuffleupagus · 2015-11-05T16:51:26Z

Looks good, thanks for the patch!

yurydelendik force-pushed the select branch from 4c46fcc to 7d6fb27 Compare November 3, 2015 20:20

timvandermeij added the text-selection label Nov 3, 2015

yurydelendik force-pushed the select branch from a25275b to 5c33de4 Compare November 4, 2015 15:22

Snuffleupagus reviewed Nov 4, 2015
View reviewed changes

yurydelendik force-pushed the select branch from 5c33de4 to fb2b765 Compare November 4, 2015 16:40

Snuffleupagus reviewed Nov 4, 2015
View reviewed changes

yurydelendik force-pushed the select branch from fb2b765 to 3f85003 Compare November 4, 2015 18:08

Rob--W reviewed Nov 4, 2015
View reviewed changes

yurydelendik force-pushed the select branch from 3f85003 to fd55c58 Compare November 4, 2015 22:26

yurydelendik force-pushed the select branch from fd55c58 to 9d0634b Compare November 4, 2015 22:51

yurydelendik force-pushed the select branch 2 times, most recently from 84c3db0 to 0951f43 Compare November 5, 2015 01:43

Non/less flashing text layer during selection.

b8ad68a

yurydelendik force-pushed the select branch from 0951f43 to b8ad68a Compare November 5, 2015 16:48

Snuffleupagus added a commit that referenced this pull request Nov 5, 2015

Merge pull request #6591 from yurydelendik/select

8079bdd

Non/less flashing text layer during selection.

Snuffleupagus merged commit 8079bdd into mozilla:master Nov 5, 2015

yurydelendik mentioned this pull request Nov 5, 2015

Improving the way how text selection works #4843

Closed

This was referenced Nov 5, 2015

Selection end becomes the start of page when not directly over text #2542

Closed

Multiline text blocks #4629

Closed

Improved Text Selection By Padding Text Elements #5545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non/less flashing text layer during selection. #6591

Non/less flashing text layer during selection. #6591

yurydelendik commented Nov 3, 2015

timvandermeij commented Nov 3, 2015

Rob--W commented Nov 3, 2015

yurydelendik commented Nov 3, 2015

yurydelendik commented Nov 3, 2015

Rob--W commented Nov 3, 2015

yurydelendik commented Nov 3, 2015

Rob--W commented Nov 3, 2015

yurydelendik commented Nov 4, 2015

Rob--W commented Nov 4, 2015

Snuffleupagus commented Nov 4, 2015

yurydelendik commented Nov 4, 2015

yurydelendik commented Nov 4, 2015

Snuffleupagus Nov 4, 2015

yurydelendik Nov 4, 2015

Snuffleupagus commented Nov 4, 2015

Snuffleupagus Nov 4, 2015

timvandermeij commented Nov 4, 2015

pdfjsbot commented Nov 4, 2015

pdfjsbot commented Nov 4, 2015

Rob--W Nov 4, 2015

Rob--W commented Nov 4, 2015

yurydelendik commented Nov 4, 2015

Rob--W commented Nov 4, 2015

yurydelendik commented Nov 4, 2015

Rob--W commented Nov 4, 2015

yurydelendik commented Nov 4, 2015

pdfjsbot commented Nov 4, 2015

pdfjsbot commented Nov 4, 2015

Rob--W commented Nov 4, 2015

Snuffleupagus commented Nov 5, 2015

Non/less flashing text layer during selection. #6591

Non/less flashing text layer during selection. #6591

Conversation

yurydelendik commented Nov 3, 2015

timvandermeij commented Nov 3, 2015

Rob--W commented Nov 3, 2015

yurydelendik commented Nov 3, 2015

yurydelendik commented Nov 3, 2015

Rob--W commented Nov 3, 2015

yurydelendik commented Nov 3, 2015

Rob--W commented Nov 3, 2015

yurydelendik commented Nov 4, 2015

Rob--W commented Nov 4, 2015

Snuffleupagus commented Nov 4, 2015

yurydelendik commented Nov 4, 2015

yurydelendik commented Nov 4, 2015

Snuffleupagus Nov 4, 2015

Choose a reason for hiding this comment

yurydelendik Nov 4, 2015

Choose a reason for hiding this comment

Snuffleupagus commented Nov 4, 2015

Snuffleupagus Nov 4, 2015

Choose a reason for hiding this comment

timvandermeij commented Nov 4, 2015

pdfjsbot commented Nov 4, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Nov 4, 2015

From: Bot.io (Linux)

Success

Published

Rob--W Nov 4, 2015

Choose a reason for hiding this comment

Rob--W commented Nov 4, 2015

yurydelendik commented Nov 4, 2015

Rob--W commented Nov 4, 2015

yurydelendik commented Nov 4, 2015

Rob--W commented Nov 4, 2015

yurydelendik commented Nov 4, 2015

pdfjsbot commented Nov 4, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Nov 4, 2015

From: Bot.io (Windows)

Success

Published

Rob--W commented Nov 4, 2015

Snuffleupagus commented Nov 5, 2015