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

Investigate if we can remove "transitionend" event listener #17347

Closed
calixteman opened this issue Nov 28, 2023 · 3 comments · Fixed by #17360
Closed

Investigate if we can remove "transitionend" event listener #17347

calixteman opened this issue Nov 28, 2023 · 3 comments · Fixed by #17360
Assignees
Labels

Comments

@calixteman
Copy link
Contributor

The bug described in https://bugzilla.mozilla.org/show_bug.cgi?id=1866773 is because at some point a blur event is triggered by:

pdf.js/web/app.js

Lines 687 to 695 in 4bf7ff2

appConfig.mainContainer.addEventListener(
"transitionend",
function (evt) {
if (evt.target === /* mainContainer */ this) {
eventBus.dispatch("resize", { source: this });
}
},
true
);

This code has been added a while back:
#1614

If I understand correctly the idea was to resize to main container once the transition for the sidebar resize is done.
The transitionend event is triggered for whatever transition we've in the main container, so for example when we're searching a word in the pdf, the event is triggered (because of the spinner).
I'd be in favor to remove this listener because it doesn't make sense to trigger a resize for whatever transition and we should figure out how to resize correctly the main container without such an event.

@Snuffleupagus, any ideas, opinions ?

@Snuffleupagus
Copy link
Collaborator

If I understand correctly the idea was to resize to main container once the transition for the sidebar resize is done.

Yes, it's intended to trigger rendering in the viewer (for the "named" zoom value) when the sidebar has been opened/closed.

We obviously need to keep that working, and another possibility would be to use an already existing ResizeObserver to trigger re-rendering. However, the only annoying thing would be that the ResizeObserver also fires while the sidebar is opening/closing and we really don't want to trigger re-rendering for all those "intermediate" events. We could perhaps use a timeout to avoid triggering re-sizing until things have settled down; I'll try to do some experiments later this week.

@Snuffleupagus Snuffleupagus self-assigned this Nov 28, 2023
@Snuffleupagus
Copy link
Collaborator

The transitionend event is triggered for whatever transition we've in the main container, so for example when we're searching a word in the pdf, the event is triggered (because of the spinner).

Using the latest Nightly version, on Windows 11, I cannot actually reproduce this; maybe it's different on other OSes?

@calixteman
Copy link
Contributor Author

Yes Emilio fixed the bug so it isn't a concern anymore.
But, for me at least, the bug highlights this use of transitionend and I think we shouldn't use it, so it's why I filed this issue.

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 a pull request may close this issue.

2 participants