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

[Editor] Add a floating button close to the selected text to highlight it (bug 1867742) #17732

Merged
merged 1 commit into from
Mar 12, 2024
Merged

[Editor] Add a floating button close to the selected text to highlight it (bug 1867742) #17732

merged 1 commit into from
Mar 12, 2024

Conversation

calixteman
Copy link
Contributor

For now keep this feature behind a pref in order to make some experiments before deciding to enable it.

@calixteman calixteman requested a review from a team as a code owner February 26, 2024 18:15
Snuffleupagus
Snuffleupagus requested changes Feb 28, 2024
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a couple of comments based on a very brief look.

Also, on a personal level, I'm finding this new feature quite annoying since the button feels "in the way" whenever I just want to select a piece of text; do we really "need" to add this?
For me bug 1868753 is a lot less "invasive", although it's obviously less discoverable.

All other editing features are simply toolbar buttons that can be easily ignored, but this feature will get "in your face" a lot more. (For me, if this feature is enabled unconditionally, I'd probably run Firefox with pdfjs.annotationEditorMode = -1 set.)

src/display/editor/tools.js Outdated Show resolved Hide resolved
src/display/editor/tools.js Outdated Show resolved Hide resolved
web/annotation_editor_layer_builder.css Outdated Show resolved Hide resolved
@marco-c
Copy link
Contributor

marco-c commented Feb 28, 2024

Also, on a personal level, I'm finding this new feature quite annoying since the button feels "in the way" whenever I just want to select a piece of text; do we really "need" to add this? For me bug 1868753 is a lot less "invasive", although it's obviously less discoverable.

All other editing features are simply toolbar buttons that can be easily ignored, but this feature will get "in your face" a lot more. (For me, if this feature is enabled unconditionally, I'd probably run Firefox with pdfjs.annotationEditorMode = -1 set.)

This is why we want to put it behind a pref and run an experiment to see how average users like it.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Feb 28, 2024 •
edited
Loading

This is why we want to put it behind a pref and run an experiment to see how average users like it.

Will that actually tell if users "like" the feature (since it seems to me that'd essentially require interviewing people and asking them), or will it simply be measuring usage?

@marco-c
Copy link
Contributor

marco-c commented Feb 28, 2024

This is why we want to put it behind a pref and run an experiment to see how average users like it.

Will that actually tell if users "like" the feature (since it seems to me that'd essentially require interviewing people and asking them), or will it simply be measuring usage?

UX did interview people and they seemed to like the feature, but we want to make sure there are only positive and no adverse effects on usage metrics (how frequently people read PDFs, how frequently people set Firefox as default PDF reader, and so on).

@calixteman calixteman force-pushed the editor_highlight_floating_button branch 2 times, most recently from 317e60c to 513c331 Compare March 4, 2024 21:08
@calixteman calixteman force-pushed the editor_highlight_floating_button branch 2 times, most recently from e199bad to 5426f51 Compare March 12, 2024 09:26
Snuffleupagus
Snuffleupagus requested changes Mar 12, 2024
View reviewed changes
src/display/editor/toolbar.js Show resolved Hide resolved
web/annotation_editor_layer_builder.css Outdated Show resolved Hide resolved
@calixteman calixteman force-pushed the editor_highlight_floating_button branch from 5426f51 to aa0b861 Compare March 12, 2024 13:15
Snuffleupagus
Snuffleupagus approved these changes Mar 12, 2024
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me, under "protest" :-)

While the implementation obviously seems fine, I'm finding the actual feature itself annoying and wish that we won't force this behaviour unconditionally on users.

@calixteman
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 1

Live output at: http://54.193.163.58:8877/1277aa8cf30b364/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/4168f94d6c6080f/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/4168f94d6c6080f/output.txt

Total script time: 6.41 mins

  • Integration Tests: FAILED

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/1277aa8cf30b364/output.txt

Total script time: 15.43 mins

  • Integration Tests: Passed

@calixteman
[Editor] Add a floating button close to the selected text to highligh…
b4267cd 
…t it (bug 1867742)

For now keep this feature behind a pref in order to make some experiments before
deciding to enable it.
@calixteman calixteman force-pushed the editor_highlight_floating_button branch from aa0b861 to b4267cd Compare March 12, 2024 14:06
@calixteman
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/4b5da38e064d3e5/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/6fefb183160b396/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/4b5da38e064d3e5/output.txt

Total script time: 6.47 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/6fefb183160b396/output.txt

Total script time: 19.08 mins

  • Integration Tests: Passed

@calixteman calixteman merged commit a7d47af into mozilla:master Mar 12, 2024
10 checks passed
@calixteman calixteman deleted the editor_highlight_floating_button branch March 12, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flodolo flodolo flodolo approved these changes

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
editor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@calixteman @marco-c @Snuffleupagus @moz-tools-bot @flodolo