Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expanding divs to improve selection. #6663

Closed
wants to merge 1 commit into from

Conversation

yurydelendik
Copy link
Contributor

Proposing a naive div boundary expansion algorithm as alternative solution for #5545. It attempts to expand axis aligned boundary on any div (rotated or not) horizontally then vertically. It's somewhat greedy scanline algorithm, which runs from left to right and extends boundary to maximum possible distance. It's trying to be fair and divides distance equally between neighbors. Some expansion may overlay if boundaries are overlay, but that shall not cause any selection issue (tab order is taking in account). The same process will be repeat from top to bottom, but on already extended boundaries.

TODOs:

  • replace slow search(es) with red-black tree
  • test and analyze with different PDFs (rtl, vertical, and with broken text order)
  • remove debug statements (css and mouse binding)
  • expand on mouse click (?)

This change is Reviewable

@yurydelendik
Copy link
Contributor Author

/cc @speedplane and @mitar for feedback

/re #4843

@yurydelendik yurydelendik force-pushed the fairexpand branch 2 times, most recently from b8c8d55 to 1aac073 Compare November 19, 2015 16:07
@yurydelendik
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

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

@mitar
Copy link
Contributor

mitar commented Nov 19, 2015

Can you explain a bit more about difference to #5545?

@mitar
Copy link
Contributor

mitar commented Nov 19, 2015

cc @nschloe who is more active on this at the moment.

@yurydelendik
Copy link
Contributor Author

Can you explain a bit more about difference to #5545?

Mostly the way how it deals with overlapping divs and distributing padding between neighbors. The #5545 might keeps gaps. Also, better handling of the rotated text.

@nschloe
Copy link
Contributor

nschloe commented Nov 19, 2015

Thanks @yurydelendik for the PR! It does improve on the flickering that PDF.js text selection currently suffers from. Also, the background color setting background-color: rgba(180, 0, 170, 0.3); makes debugging easy. With a random PDF

ex

I find that when hovering of a "white" area, the text selection will still jump.

@timvandermeij
Copy link
Contributor

Especially text selection just outside of text has improved massively. With the current master and the Tracemonkey paper, starting selection just before "Abstract" makes all text above it selected too. With this patch that does not happen anymore and just "Abstract" is selected, which is also what I would expect. If we can make this more efficient (you also mention this in the description/comments) I think this would be a great improvement.

@yurydelendik
Copy link
Contributor Author

... With a random PDF

The white spots on the right are caused by the subscript text on the previous line caused by special characters at the end of the previous line (boxes are on top each other and tab index of top one is less than bottom). The white spot(s) on the left are not present on FF (cause by Chrome returning 0 for ctx.measureText('\u0014').width, also means that there are non-readable character in the text layer and you cannot really copy it in other viewers.

@brendandahl
Copy link
Contributor

This does have the side affect of always showing the text cursor no matter where you hover over the page (instead of just over text).

@yurydelendik
Copy link
Contributor Author

This does have the side affect of always showing the text cursor no matter where you hover over the page

Shall we wrap span-in-div? But I would recommend that we will turn this mode per demand: on mouse click and/or via preference. ("On mouse click"-expand will solve cursor issue) So far only non-Firefox browsers need this mode.

@tonyjin
Copy link

tonyjin commented Feb 25, 2016

👍 Really looking forward to this landing :)

@speedplane
Copy link

I played with this a bit, it looks really good, and a whole lot simpler than my approach with crazy quad trees. It even works on rotated text (e.g., http://mpdf1.com/examples/example_RTL.pdf), really impressive!

Is there an O(N^2) issue in there (I assume so due to the first item in your TODO)?

@nschloe
Copy link
Contributor

nschloe commented Apr 1, 2016

Anything I can do to help getting this PR into shape?

jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 16, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option
3. Moving feature functionality from text_layer_builder.js to text_layer.js
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 16, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option
3. Moving feature functionality from text_layer_builder.js to text_layer.js
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 16, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option
3. Moving feature functionality from text_layer_builder.js to text_layer.js
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 17, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option
3. Moving feature functionality from text_layer_builder.js to text_layer.js
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 17, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option
3. Moving feature functionality from text_layer_builder.js to text_layer.js
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 17, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option
3. Moving feature functionality from text_layer_builder.js to text_layer.js
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 17, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option
3. Moving feature functionality from text_layer_builder.js to text_layer.js
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 18, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added enhanceOnClick option to textLayerBuilder. True by default.
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 19, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added enhanceOnClick option to textLayerBuilder. True by default.
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 20, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added enhanceOnClick option to textLayerBuilder. True by default.
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 22, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added expandTextDivs method to only load expanded divs on first click, and only show on subsequent clicks
@yurydelendik
Copy link
Contributor Author

See #7539

jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 23, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added expandTextDivs method to only load expanded divs on first click, and only show on subsequent clicks
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 23, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added expandTextDivs method to only load expanded divs on first click, and only show on subsequent clicks
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 23, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added expandTextDivs method to only load expanded divs on first click, and only show on subsequent clicks
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 23, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added expandTextDivs method to only load expanded divs on first click, and only show on subsequent clicks
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 23, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added expandTextDivs method to only load expanded divs on first click, and only show on subsequent clicks
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 23, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added expandTextDivs method to only load expanded divs on first click, and only show on subsequent clicks
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 24, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added expandTextDivs method to only load expanded divs on first click, and only show on subsequent clicks
jeremypress pushed a commit to jeremypress-forks/pdf.js that referenced this pull request Aug 31, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added expandTextDivs method to only load expanded divs on first click, and only show on subsequent clicks
darshitvvora pushed a commit to darshitvvora/pdf.js that referenced this pull request Sep 13, 2016
1. Expanding divs to improve text selection. (Yury)
2. Adding enhanceTextSelection as an option.
3. Moving feature functionality from text_layer_builder.js to text_layer.js.
4. Added expandTextDivs method to only load expanded divs on first click, and only show on subsequent clicks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants