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.

Sign up for GitHub

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

Jump to bottom

Enable enhanced text selection by default #7584

Closed
3 of 5 tasks
timvandermeij opened this issue Sep 1, 2016 · 1 comment
Closed
3 of 5 tasks

Enable enhanced text selection by default #7584

timvandermeij opened this issue Sep 1, 2016 · 1 comment
Labels
performance text-selection

Comments

@timvandermeij
Copy link
Contributor

timvandermeij commented Sep 1, 2016 •
edited
Loading

With #7539 merged, we should address the following items before we can enable the feature by default:

  • Use a red-black tree or binary search in expandBoundsLTR (see inline TODO item)
  • Use reference counting in expandBoundsLTR (see inline TODO item)
  • Remove dataset usage in text_layer.js as there is unnecessary type conversion
  • Improve handling cases where each glyph is in a separate div (see [api-minor] Expanding divs to improve selection #7539 (comment))
  • Make enhance text selection a viewer preference
@Snuffleupagus Snuffleupagus mentioned this issue Sep 2, 2016
Merged
@timvandermeij timvandermeij mentioned this issue Sep 2, 2016
Merged
This was referenced Sep 7, 2016
Merged
Merged
@Snuffleupagus Snuffleupagus mentioned this issue Feb 13, 2017
Closed
@Rob--W Rob--W mentioned this issue Feb 18, 2018
Merged
Rob--W added a commit to Rob--W/pdf.js that referenced this issue Feb 21, 2018
@Rob--W
Clearly mark "enhanced text selection" as "experimental"
d491c45 
The "enhanced text selection" mode is still experimental,
so clearly mark it as experimental to signal that there may be bugs.

The list of tasks that block promotion to stable is at mozilla#7584.
Rob--W added a commit to Rob--W/pdf.js that referenced this issue Feb 22, 2018
@Rob--W
[CRX] Make textLayerMode pref visible and add migration logic
94a49fa 
In a1cfa5f, the textLayerMode
preference was introduced, to replace the disableTextLayer and
enhanceTextSelection preferences.

As a result, the text selection preference was no longer visible
in Chrome (because preferences are only rendered by default for
boolean preferences, not for enumerations).

This commit adds the necessary bits to
extensions/chromium/options/options.{html,js}
so that the textLayerMode preference can be changed again.

Also, migration logic has been added to move over preferences
from the old to the new names:
- In web/chromecom.js, the logic is added to translate
  preferences that were set by an administrator (it is read-only,
  so this layer is unavoidable).
- In extensions/chromium/options/migration.js, similar logic is
  added, except in this case the preference storage is writable,
  so this migration logic happens only once.

The "enhanced text selection" mode is still experimental, so it
has been marked as experimental to signal that there may be bugs.
The list of tasks that block promotion to stable is at mozilla#7584.
ltetzlaff pushed a commit to ltetzlaff/pdf.js that referenced this issue Apr 24, 2018
@Rob--W @ltetzlaff
[CRX] Make textLayerMode pref visible and add migration logic
16a14b0 
In a1cfa5f, the textLayerMode
preference was introduced, to replace the disableTextLayer and
enhanceTextSelection preferences.

As a result, the text selection preference was no longer visible
in Chrome (because preferences are only rendered by default for
boolean preferences, not for enumerations).

This commit adds the necessary bits to
extensions/chromium/options/options.{html,js}
so that the textLayerMode preference can be changed again.

Also, migration logic has been added to move over preferences
from the old to the new names:
- In web/chromecom.js, the logic is added to translate
  preferences that were set by an administrator (it is read-only,
  so this layer is unavoidable).
- In extensions/chromium/options/migration.js, similar logic is
  added, except in this case the preference storage is writable,
  so this migration logic happens only once.

The "enhanced text selection" mode is still experimental, so it
has been marked as experimental to signal that there may be bugs.
The list of tasks that block promotion to stable is at mozilla#7584.
movsb pushed a commit to movsb/pdf.js that referenced this issue Jul 14, 2018
@Rob--W
[CRX] Make textLayerMode pref visible and add migration logic
46402d5 
In a1cfa5f, the textLayerMode
preference was introduced, to replace the disableTextLayer and
enhanceTextSelection preferences.

As a result, the text selection preference was no longer visible
in Chrome (because preferences are only rendered by default for
boolean preferences, not for enumerations).

This commit adds the necessary bits to
extensions/chromium/options/options.{html,js}
so that the textLayerMode preference can be changed again.

Also, migration logic has been added to move over preferences
from the old to the new names:
- In web/chromecom.js, the logic is added to translate
  preferences that were set by an administrator (it is read-only,
  so this layer is unavoidable).
- In extensions/chromium/options/migration.js, similar logic is
  added, except in this case the preference storage is writable,
  so this migration logic happens only once.

The "enhanced text selection" mode is still experimental, so it
has been marked as experimental to signal that there may be bugs.
The list of tasks that block promotion to stable is at mozilla#7584.
@timvandermeij timvandermeij mentioned this issue Oct 3, 2019
Closed
@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Aug 28, 2021 •
edited
Loading

I'm proposing that we close this as WONTFIX for the time being, since:

  • No real work, to address the remaining points, have happened for a couple years and the enhanceTextSelection-mode is essentially not being worked on or improved at all.

  • Even with the aforementioned improvements implemented (which only affects the initial parsing), the enhanceTextSelection-mode still won't perform well during actual user-interaction for pages that contain a lot of textLayer-elements.
    The explanation here is the need to update the DOM as the user clicks/releases the mouse button, for potentially thousands of elements, which unfortunately will feel sluggish and result in a bad UX in larger and/or complex PDF documents.

@Snuffleupagus Snuffleupagus mentioned this issue Jan 17, 2022
Closed
@Snuffleupagus Snuffleupagus mentioned this issue Jul 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
performance text-selection
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@timvandermeij @Snuffleupagus