Using the browser's Back and Forward buttons doesn't update the media library

[Faison]

In the Media Library (grid view), there's some backbone router magic going on to update the URL as you do things like click on an image to bring up the attachment details modal, or close the attachment details modal. Today, I noticed that after I open the attachment details for an image, I can press the browser's "back" button and the URL updates, but the attachment details are still visible.

How to reproduce

• Log into WordPress and go to the Media Library
• Ensure that you're looking at the grid view
• Click on an image to bring up the "Attachment Details" modal and notice that the URL updates to something like 'example.com/wp-admin/upload.php?item=1'
• Click your browser's "Back" button and notice that the URL updates to something like 'example.com/wp-admin/upload.php', but the "Attachment Details" modal is still on the screen.

I tested this in a fresh install for versions 4.0, 4.1.1, and trunk (a week old at most) in Chrome, Chromium and Firefox.

I'd love to provide a patch, BUT I barely understand enough of the code to follow how the media grid is created.

One Additional Note: After following the instructions listed above, click the browser's "Forward" button so that the URL returns to something like 'example.com/wp-admin/upload.php?item=1', and you'll get the following error in Chrome/Chromium's Dev console:

Uncaught TypeError: Cannot read property 'findWhere' of undefined -- media-grid.js?ver=4.1.1:335

[DrewAPicture]

For what it's worth, this behavior has been intact since the media grid was introduced in 3.9, so definitely not a regression. Let's take another look in 4.3.

[wonderboymusic]

In 32466:

Media Grid: clean up our flawed Backbone.Router implementation so that the browser forward/back buttons work the same way as the left/right keys when the Edit Attachment frame is open.

Fixes #31846.

[wonderboymusic]

This router implementation kinda sucks and there are edge case bugs everywhere

[wonderboymusic]

Long story short: the Router is broken, I will fix it.

[obenland]

Replying to wonderboymusic:

Long story short: the Router is broken, I will fix it.

Do you think you can get it fixed before beta3? Or should we do it in a new ticket for 4.4?

[wonderboymusic]

I'll make a decision in the next 24 hours

[obenland]

@wonderboymusic, what's the status here? Revert and punt?

[wonderboymusic]

[33342]

[kucrut]

Fix media grid's router

[kucrut]

What 31846.diff​ does:

• Adds a new reset route (upload.php) that closes the frame.
• Fixes the showItem route to make sure clicking the back/forward button also updates the frame content.
• Fixes the delete button to make sure the frame is closed when the attachment being edited is deleted (previously the frame only closes when you delete the initial attachment loaded).
• Fixes the search handler to make sure the address and the search input value are in sync and prevents it from modifying the address when you initially load the page with a specified item.
• Store both frames in wp.media.frames to avoid instantiating multiple edit frames.

[adamsilverstein]

@kucrut very nice! thanks for your patch. I tested a little bit and verified the issue described in the ticket (as well as the others you mentioned) is corrected. One new issue I see in testing: do a search, then click an item to see the detail view, then hit the back button. this should take you back to your search results, instead its going back to the original upload screen.

Care to work on a revised patch to address this issue? Thanks!

31846.2.diff​ is just a refresh of your original patch against trunk.

[kucrut]

Thanks for testing out the patch @adamsilverstein!

In 31846.3.diff​:

• Close modal when navigating to search.
• Carefully reset route when the modal close button is clicked by checking the search field.

[adamsilverstein]

I tested this out again and it works well for me. This covers most of the routes you can navigate to (the edit mode is missing). I would appreciate some additional eyes and testing on this, it seems like a huge improvement to me.

[adamsilverstein]

Here is a screencast showing the router functionality after the latest patch - ​https://cl.ly/0M1c3Q0Z181z

[adamsilverstein]

31846.4.diff​ Add support for routing to detail edit mode.

[adamsilverstein]

31846.5.diff​ fix a double routing issue when loading the edit route

[joemcgill]

@adamsilverstein I'm seeing some weird behavior when navigating through browser history with the media frame open. Here's a screencast of what I'm seeing.

​https://cloudup.com/cN7oyMKcUE1

[adamsilverstein]

Replying to joemcgill:

@adamsilverstein I'm seeing some weird behavior when navigating through browser history with the media frame open. Here's a screencast of what I'm seeing.

​https://cloudup.com/cN7oyMKcUE1

Thanks for testing @joemcgill, I will review the screencast. Was there any particular step you took that seemed to mess up the history?

[joemcgill]

Replying to adamsilverstein:

Thanks for testing @joemcgill, I will review the screencast. Was there any particular step you took that seemed to mess up the history?

The screencast should show the issues, but generally moving back and forth through history using back/forward browser buttons is what caused problems. Forward seemed to be the most problematic, as often times the URL would reset back to the normal media library URL when moving forward (i.e., no query params) and then the frame and route history would get out of sync.

[adamsilverstein]

31846.6.diff​:

Correct an issue that corrupted browser history by adding multiple references to the root route.

• Use { replace: true } when resetting route for close/search so history stays clean
• switch from _.debounce to _.throttle so url changes don't get delayed, preventing a re-navigation when you changed urls quickly.

@joemcgill can you please give it another test run?

Thanks.

[joemcgill]

Replying to adamsilverstein:

@joemcgill can you please give it another test run?

31846.6.diff​ is working much better with browser history and I've not seen any problems navigating through several different contexts using browser history. Good work!

[adamsilverstein]

Too late to land this in 4.8, punting.

[afercia]

Just noticed an edge case, not strictly related to this patch though. Next/previous navigation happens also when using the keyboard Arrow right/left keys. This might trigger a security error in Safari. To reproduce:

• use Safari 10 or Safari Technology Preview
• ensure you have 30-40 attachments in your media library to make testing easier
• click an attachment, the edit attachment modal opens
• press and keep pressed the Arrow right key on your keyboard
• navigation to the next item is triggered continuously until the last item
• when on the last item, press and keep pressed the Arrow left key until you reach the first item
• go back and forth this way until Safari triggers this error:

SecurityError (DOM Exception 18): Attempt to use history.pushState() more than 100 times per 30.000000 seconds

So the number of attachments and browsing direction is not relevant to trigger the security error, all that's needed is to call history.pushState() more than 100 times per 30 seconds. Googling a bit, it appears to be "by design" to prevent a potential security bug abusing pushState/replaceState. Some references:
​https://forums.developer.apple.com/thread/36650
Seems this restriction was initially 100 calls, then softened to 100 calls per 30 seconds:
​https://trac.webkit.org/changeset/198687/webkit/trunk/Source/WebCore/page/History.cpp

Out of curiosity, I've checked what happens on the Theme installer and noticed the arrow navigation there uses keyup instead of keydown so the "continuous triggering" doesn't happen: users have to release the pressed key to trigger navigation. Wondering if also the attachments navigation should use keyup instead of keydown, also for consistency.

P.S. not sure if the change to Gruntfile.js should stay in 31846.6.diff​

[adamsilverstein]

Replying to afercia:

Out of curiosity, I've checked what happens on the Theme installer and noticed the arrow navigation there uses keyup instead of keydown so the "continuous triggering" doesn't happen: users have to release the pressed key to trigger navigation. Wondering if also the attachments navigation should use keyup instead of keydown, also for consistency.

I think you are right, we should change this input event to make it more consistent. Since we are no longer supporting older IE browsers, we can probably use ​the input event everywhere. I can create a separate ticket for this task.

P.S. not sure if the change to Gruntfile.js should stay in 31846.6.diff​

Right, I will remove that - thanks for catching.

[adamsilverstein]

In 41021:

Media: library grid view - improve browser history support.

Set view state properly when navigating history using the browser back/next button in the media library (grid view). Correctly handle navigating, search, image detail view and image edit mode. Also handle bookmarking/reloading.

Props kucrut, joemcgill, afercia.
Fixes #31846.