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

Introduce a --viewer-container-height CSS variable to simplify the code #14880

Merged

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented May 5, 2022

This new CSS variable will allow us to simplify a couple of different viewer components, since we no longer need to use JavaScript-based hacks and can directly set the CSS rules instead. In particular:

  • The BaseViewer-handling, used as part of the code that will center pages vertically in PresentationMode, can be simplified.
    By using CSS to control the height of the "dummy"-page we avoid unnecessarily invalidating the scale-value, which can reduce some unneeded re-rendering while PresentationMode is active.

  • The SecondaryToolbar.#setMaxHeight-method, and its associated parameters, are no longer necessary and can be completely removed.

Note that in order for things to work correctly in general, the new --viewer-container-height CSS variable must potentially be updated on any window-based "resize"-event (even when there's no zooming). While this is currently only done in the default viewer, that shouldn't be an issue since neither PresentationMode nor Toolbar-functionality is included in the "viewer components".

@Snuffleupagus Snuffleupagus marked this pull request as draft May 6, 2022 07:37
@Snuffleupagus Snuffleupagus force-pushed the CSS--viewer-container-height branch 5 times, most recently from 05f829e to 905fce6 Compare May 6, 2022 11:34
…code

This new CSS variable will allow us to simplify a couple of different viewer components, since we no longer need to use JavaScript-based hacks and can directly set the CSS rules instead. In particular:

 - The `BaseViewer`-handling, used as part of the code that will center pages *vertically* in PresentationMode, can be simplified.
   By using CSS to control the height of the "dummy"-page we avoid unnecessarily invalidating the scale-value, which can reduce *some* unneeded re-rendering while PresentationMode is active.

 - The `SecondaryToolbar.#setMaxHeight`-method, and its associated parameters, are no longer necessary and can be completely removed.

Note that in order for things to work correctly in general, the new `--viewer-container-height` CSS variable must potentially be updated on any window-based "resize"-event (even when there's no zooming). While this is currently only done in the default viewer, that shouldn't be an issue since neither PresentationMode nor Toolbar-functionality is included in the "viewer components".
@Snuffleupagus Snuffleupagus marked this pull request as ready for review May 6, 2022 13:55
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented May 6, 2022

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/052e859636ab2c9/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 6, 2022

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/052e859636ab2c9/output.txt

Total script time: 2.61 mins

Published

@timvandermeij timvandermeij merged commit 82353cf into mozilla:master May 7, 2022
@timvandermeij
Copy link
Contributor

Seems to work just fine; nice clean-up!

@Snuffleupagus Snuffleupagus deleted the CSS--viewer-container-height branch May 7, 2022 10:07
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.

None yet

3 participants