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

[Editor] Avoid to darken the current editor when opening the alt-text dialog #17002

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

calixteman
Copy link
Contributor

No description provided.

@calixteman calixteman changed the title [Editor] Avoid to darken the current editor when open the alt-text dialog [Editor] Avoid to darken the current editor when opening the alt-text dialog Sep 21, 2023
@calixteman
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

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

Total script time: 1.49 mins

Published

@calixteman
Copy link
Contributor Author

We should disable zooming with the mouse when the dialog is displayed because it leads to some inconsistencies.

@Snuffleupagus
Copy link
Collaborator

We should disable zooming with the mouse when the dialog is displayed because it leads to some inconsistencies.

That'd hopefully be as simple as adding e.g. the following code at the appropriate spots in the various wheel/touch event handlers in the web/app.js file:

if (PDFViewerApplication.overlayManager.active) {
  evt.preventDefault();
  return;
}

@Snuffleupagus
Copy link
Collaborator

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/2dabbc852a819d5/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/2dabbc852a819d5/output.txt

Total script time: 1.58 mins

Published

@Snuffleupagus
Copy link
Collaborator

This looks much nicer/better, since it clearly focuses on the active image!

One small question, although I'd guess that it's a wontfix since it's an edge-case that might be annoying to fix in general:
If you add a fairly large image, I used this one, such that the entire image won't fit on screen at once, when opening the altText dialog the mask may also "include" part of the viewer toolbar.

toolbar-overlay

@calixteman
Copy link
Contributor Author

This looks much nicer/better, since it clearly focuses on the active image!

One small question, although I'd guess that it's a wontfix since it's an edge-case that might be annoying to fix in general: If you add a fairly large image, I used this one, such that the entire image won't fit on screen at once, when opening the altText dialog the mask may also "include" part of the viewer toolbar.

Yep I thought about this case. A way to solve it, would be to take into account only the really visible part of the editor which should be possible but probably more work (we should take into account the toolbar, margins, ...) or maybe there's a smart trick we could use.

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, this looks great; thank you!

@calixteman calixteman merged commit 2fc8ab3 into mozilla:master Sep 22, 2023
@calixteman calixteman deleted the alt_text_mask branch September 22, 2023 10:17
calixteman added a commit to calixteman/pdf.js that referenced this pull request Sep 22, 2023
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.

3 participants