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

Implement caret browsing mode (bug 807730) #17611

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

calixteman
Copy link
Contributor

The users will be able to navigate within the pdf in using the arrows and they'll be able to select some text, for example in order to highlight it.

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.

Sorry, but I'd really like to see a solution to issue #17561 before we add even more functionality on top of this; please see the example added in #17561 (comment).

web/app.js Outdated Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

Sorry, but I'd really like to see a solution to issue #17561 before we add even more functionality on top of this; please see the example added in #17561 (comment).

As said in the mentioned bug, I've a solution for it, but this feature has a higher priority.

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/questions, but I've not yet had time to really look at the CaretBrowsingMode-implementation in any detail.

Also, does this work correctly in RTL mode? Can be tested e.g. with http://localhost:8888/web/viewer.html#locale=he

web/app.js Outdated Show resolved Hide resolved
web/app.js Outdated Show resolved Hide resolved
web/app.js Show resolved Hide resolved
web/caret_browsing.js Outdated Show resolved Hide resolved
web/caret_browsing.js Outdated Show resolved Hide resolved
web/caret_browsing.js Outdated Show resolved Hide resolved
web/caret_browsing.js Outdated Show resolved Hide resolved
web/caret_browsing.js Outdated Show resolved Hide resolved
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.

Did you, successfully, test this in RTL mode?

test/integration/caret_browsing_spec.mjs Outdated Show resolved Hide resolved
web/app.js Outdated Show resolved Hide resolved
web/caret_browsing.js Outdated Show resolved Hide resolved
test/integration/caret_browsing_spec.mjs Show resolved Hide resolved
web/caret_browsing.js Outdated Show resolved Hide resolved
web/caret_browsing.js Show resolved Hide resolved
web/caret_browsing.js Outdated Show resolved Hide resolved
web/caret_browsing.js Outdated Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

Did you, successfully, test this in RTL mode?

Yep I ran with locale=he and a pdf in hebrew and I think everything is working correctly (my skills in hebrew are close to null it's why "I think...").

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, assuming that:

@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/caad736aef74fcd/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

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

Total script time: 25.09 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 16
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/4a53ac83fe6bfa8/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/caad736aef74fcd/output.txt

Total script time: 41.85 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 2

Image differences available at: http://54.193.163.58:8877/caad736aef74fcd/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

/botio-windows integrationtest

@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/979a71d61328874/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/979a71d61328874/output.txt

Total script time: 16.12 mins

  • Integration Tests: FAILED

The users will be able to navigate within the pdf in using the arrows
and they'll be able to select some text, for example in order to
highlight it.
@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/7ba383a9ddadee7/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/5b63aa4bed38c86/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/7ba383a9ddadee7/output.txt

Total script time: 6.09 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/5b63aa4bed38c86/output.txt

Total script time: 17.68 mins

  • Integration Tests: Passed

@calixteman
Copy link
Contributor Author

The test failure on the windows bot was due to the EOL (\r\n instead of \n), it's fixed now.

@calixteman calixteman merged commit 60fd9d5 into mozilla:master Feb 7, 2024
7 checks passed
@calixteman calixteman deleted the caret_browsing_mode branch February 7, 2024 09:16
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.

None yet

3 participants