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

Ensure that the 'pagechanging' event is triggered on load, to avoid incorrect initial button state (PR 7289 followup) #7405

Closed
wants to merge 1 commit into from
Closed

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Jun 11, 2016

With the changes in PR #7289, we no longer dispatch a 'pagechanging' event on load. Since most PDF documents open on the first page, this means that the previous and firstPage buttons are not correctly disabled.

A picture is worth a thousand words:

buttons-disabled

/cc @yurydelendik

…ncorrect initial button state (PR 7289 followup)

With the changes in PR 7289, we no longer dispatch a 'pagechanging' event on load. Since most PDF documents open on the first page, this means that the `previous` and `firstPage` buttons are not correctly disabled.
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/dbbc44bf362a39f/output.txt

@yurydelendik
Copy link
Contributor

Looks like it's a little bit confusing. Can we try to move https://github.com/mozilla/pdf.js/blob/master/web/app.js#L1956-L1962 somewhere e.g. in updateUIToolbar() function on call it, first, on viewer initialization, and then on pagechanging. (We can move more stuff there e.g. scale change or file opened)

@Snuffleupagus
Copy link
Collaborator Author

@yurydelendik Does this look like what you had in mind master...Snuffleupagus:updateUIToolbar?

@yurydelendik
Copy link
Contributor

@Snuffleupagus yes, but I'm not sure we need to provide any args to the function (can it pull all needed info from the object states)

@yurydelendik
Copy link
Contributor

Approach in #7405 (comment) looks good. We can re-iterate with further optimizations once it's landed to fix the regression.

@Snuffleupagus
Copy link
Collaborator Author

but I'm not sure we need to provide any args to the function (can it pull all needed info from the object states)

Sorry, I'm not sure that I understand where you're suggesting that we should get the values from if we're not passing them into the method!?
I figured I should ask, since if it's easy to address this comment, I might as well try and fix it before submitting the PR (less code churn that way).

@yurydelendik
Copy link
Contributor

yurydelendik commented Jun 29, 2016

Ideally, we shall have a function that if we call it, the toolbar state will be updated to the state of the model the viewer in (without knowing who calls it, but atm resetNumPages or scaleValue provide this information). I anticipate that this function will be call at any event, e.g. initializing/zooming/scrolling/document change/etc. or combination of several. It not a problem, but it will be good to move/split this method into several (for each toolbar, context menu) and call them from the viewer's.

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Jun 29, 2016

Approach in #7405 (comment) looks good. We can re-iterate with further optimizations once it's landed to fix the regression.

OK, I'm just going to submit a PR with what I've got for now, and we can improve on this later on.
Closing as superseded by PR #7458.

@Snuffleupagus Snuffleupagus deleted the ensure-pagechanging-triggered-onload branch June 29, 2016 19:14
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.

3 participants