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

Support adding pages, in addition to regular destinations, to the browser history and use it with thumbnails (issue 12440) #12493

Merged
merged 3 commits into from
Oct 20, 2020

Conversation

Snuffleupagus
Copy link
Collaborator

While the referenced issue could very well be seen as an edge-case, this patch adds support for updating of the browser history when interacting with the thumbnails in the sidebar (assuming we want to do this).

The main reason for adding the history implementation in the first place, was to simplify navigating back to a previous position in the document when named/explicit destinations are used (e.g. when clicking on "links" or when using the outline in the sidebar).
As such, it never really crossed by mind to update the browser history when the thumbnails are used. However, a user clicking on thumbnails could be regarded as a pretty strong indication of user-intent w.r.t. navigation in the document, hence I suppose that updating the browser history in this particular case probably won't hurt.

Fixes #12440

This modernizes and improves the code, by using `async`/`await` and by extracting the helper function to its own method.
To hopefully avoid confusion, given the next patch, the method is also re-named to `goToDestination` to make is slightly clearer what it actually does.
…wser history and use it with thumbnails (issue 12440)

While the referenced issue could very well be seen as an edge-case, this patch adds support for updating of the browser history when interacting with the thumbnails in the sidebar (assuming we want to do this).

The main reason for adding the history implementation in the first place, was to simplify navigating back to a previous position in the document when named/explicit destinations are used (e.g. when clicking on "links" or when using the outline in the sidebar).
As such, it never really crossed by mind to update the browser history when the thumbnails are used. However, a user clicking on thumbnails could be regarded as a pretty strong indication of user-intent w.r.t. navigation in the document, hence I suppose that updating the browser history in this particular case probably won't hurt.
There's no compelling reason to update this property *manually* in multiple places, since that's error-prone with any future code changes, given that `_updateInternalState` is always called just before anyway.
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/6f80a2d3dfa6d8e/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/6f80a2d3dfa6d8e/output.txt

Total script time: 3.91 mins

Published

@Snuffleupagus Snuffleupagus marked this pull request as ready for review October 19, 2020 09:27
@timvandermeij
Copy link
Contributor

Wouldn't we then want to do this in more situations? Clicking a thumbnail changes the page number, so then I'd expect this to also work when entering a page number in the textbox or via the hash parameters, and also when clicking an outline item. I'd say those are all signs of navigation within the document. What do you think?

@Snuffleupagus
Copy link
Collaborator Author

so then I'd expect this to also work when entering a page number in the textbox

That's somewhat more difficult to implement well, given that we support page labels in addition to just page numbers; hence that's not something I want to worry about here (or even at all).

via the hash parameters,

But, that's been supported from the start[1]...

and also when clicking an outline item.

... and so has this, since those are just destinations, given that it's the primary reason for PDFHistory existing in the first place :-)


[1] To the extent that doing so is feasible, since the old PDFHistory implementation showed that you want to steer well clear of trying to navigate back-and-forth between previous history entries in order to edit them.

@timvandermeij
Copy link
Contributor

timvandermeij commented Oct 20, 2020

But, that's been supported from the start

You're completely right; now that I tried again it clearly works. Not sure how I tried it yesterday, but sorry for the noise.

The patch seems to work just fine, so let's do this. Thanks!

@timvandermeij timvandermeij merged commit e389ed6 into mozilla:master Oct 20, 2020
@Snuffleupagus Snuffleupagus deleted the PDFHistory.pushPage branch October 21, 2020 09:39
@Snuffleupagus
Copy link
Collaborator Author

Not sure how I tried it yesterday, but sorry for the noise.

It's very possible that your initial testing actually uncovered a bug, since all of this is unfortunately pretty complicated[1]. If so, please let me know if you manage to come up with a way to reproduce it.

No worries; thanks for taking the time to test this and think about the various cases involved.


[1] Unless you simply found an edge-case, which is purposely not implemented because doing so would complicate and/or break the primary functionality of PDFHistory; it'd still be good to know though!

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.

Back action navigates to wrong page after thumbnail is clicked
3 participants