Make WordPress Core

Opened 9 years ago

Closed 7 years ago

#31846 closed defect (bug) (fixed)

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

Reported by: faison's profile Faison Owned by: adamsilverstein's profile adamsilverstein
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: has-patch commit
Focuses: javascript Cc:

Description

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

Attachments (6)

31846.diff (12.8 KB) - added by kucrut 9 years ago.
Fix media grid's router
31846.2.diff (12.2 KB) - added by adamsilverstein 8 years ago.
31846.3.diff (13.7 KB) - added by kucrut 8 years ago.
31846.4.diff (16.1 KB) - added by adamsilverstein 7 years ago.
31846.5.diff (17.7 KB) - added by adamsilverstein 7 years ago.
31846.6.diff (18.4 KB) - added by adamsilverstein 7 years ago.

Download all attachments as: .zip

Change History (39)

#1 @wonderboymusic
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.2
  • Owner set to wonderboymusic
  • Status changed from new to assigned

#2 @DrewAPicture
9 years ago

  • Focuses javascript added

#3 @DrewAPicture
9 years ago

  • Keywords 4.3-early added
  • Milestone changed from 4.2 to Future Release

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.

#4 @obenland
9 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

#5 @wonderboymusic
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#6 @wonderboymusic
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#7 follow-up: @wonderboymusic
9 years ago

  • Type changed from defect (bug) to task (blessed)

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

#8 in reply to: ↑ 7 @obenland
9 years ago

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?

#9 @wonderboymusic
9 years ago

I'll make a decision in the next 24 hours

#10 @obenland
9 years ago

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

#11 @wonderboymusic
9 years ago

  • Milestone changed from 4.3 to Future Release
Version 1, edited 9 years ago by ocean90 (previous) (next) (diff)

@kucrut
9 years ago

Fix media grid's router

#12 @kucrut
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

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.

#13 @ocean90
8 years ago

  • Type changed from task (blessed) to defect (bug)

#14 @adamsilverstein
8 years ago

@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
8 years ago

#15 @kucrut
8 years ago

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.
Last edited 8 years ago by kucrut (previous) (diff)

#16 @adamsilverstein
7 years ago

  • Milestone changed from Future Release to 4.8

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.

Last edited 7 years ago by adamsilverstein (previous) (diff)

#17 @adamsilverstein
7 years ago

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

#18 @adamsilverstein
7 years ago

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

#19 @adamsilverstein
7 years ago

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

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


7 years ago

#21 @joemcgill
7 years ago

  • Owner changed from wonderboymusic to adamsilverstein
  • Status changed from reopened to assigned

#22 follow-up: @joemcgill
7 years ago

@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

#23 in reply to: ↑ 22 ; follow-up: @adamsilverstein
7 years ago

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?

#24 in reply to: ↑ 23 @joemcgill
7 years ago

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.

#25 follow-up: @adamsilverstein
7 years ago

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.

#26 in reply to: ↑ 25 @joemcgill
7 years ago

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!

#27 @adamsilverstein
7 years ago

  • Milestone changed from 4.8 to Future Release

Too late to land this in 4.8, punting.

This ticket was mentioned in Slack in #core by obenland. View the logs.


7 years ago

#29 follow-up: @afercia
7 years ago

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

Last edited 7 years ago by afercia (previous) (diff)

#30 in reply to: ↑ 29 @adamsilverstein
7 years ago

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.

#31 @adamsilverstein
7 years ago

  • Keywords commit added; needs-testing removed

#32 @adamsilverstein
7 years ago

  • Milestone changed from Future Release to 4.9

#33 @adamsilverstein
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.