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

Expose entireWord in updateFindControlState #18303

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

bootleq
Copy link
Contributor

@bootleq bootleq commented Jun 20, 2024

This is a request to additionally expose entireWord state in updateFindControlState.

Have a use case

I am studying Firefox findbar behavior when viewing PDF, especially the "not found" sound.
The condition of the sound requires to consider whether entireWord is checked.
See related implementation in https://hg.mozilla.org/mozilla-central/rev/16b902cbcf26

This PR would help if I move the implementation from cpp to jsm.

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.

Please squash the commits, and please also ensure that the commit message contains all of the context from #18303 (comment).

web/pdf_find_controller.js Show resolved Hide resolved
@Snuffleupagus
Copy link
Collaborator

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/13a29b0a0c35890/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/959293c25a66bbf/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/959293c25a66bbf/output.txt

Total script time: 28.72 mins

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

Image differences available at: http://54.241.84.105:8877/959293c25a66bbf/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/13a29b0a0c35890/output.txt

Total script time: 44.18 mins

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

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

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, with one more (small) comment addressed; thank you!

test/unit/pdf_find_controller_spec.js Outdated Show resolved Hide resolved
@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jun 21, 2024

Please squash the commits, since we don't use separate fixup commits in this project.

Allow apps with supportsIntegratedFind to better monitor the find state.

A recognized use case is the Firefox findbar, its "not found" sound must
consider `entireWord` and only make noise when it is off.

See related implementation in
https://hg.mozilla.org/mozilla-central/rev/16b902cbcf26

This change can help if we have to move the implementation from cpp to jsm.
@timvandermeij timvandermeij merged commit f9ff613 into mozilla:master Jun 21, 2024
7 checks passed
@timvandermeij
Copy link
Contributor

Looks good to me too, especially with the added test. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants