Make WordPress Core

Opened 3 years ago

Last modified 31 hours ago

#54091 assigned defect (bug)

Attachment details pane navigation not working after opening 81st (or greater) item and refreshing the browser

Reported by: ppetrov2c's profile ppetrov2c Owned by: antpb's profile antpb
Milestone: 6.6 Priority: normal
Severity: trivial Version: 5.8
Component: Media Keywords: needs-patch dev-feedback
Focuses: ui, javascript Cc:

Description

Setup:

  • clean 5.8;
  • twentytwentyone active;
  • no plugins active.

Steps to reproduce:

  • open media page;
  • click on the 41st item in the list;
  • notice the left arrow is enabled (which is to be expected, since there are previous items);
  • refresh the page.

Notice the left arrow now - it's disabled.

Not sure if this is intended, but since the item isn't loaded (in the grid), there's no previous but there's only next, which points to the first item. Clicking the right arrow goes to the it and you can't go back to the previous item.

Attachments (1)

54091.patch (517 bytes) - added by antpb 4 months ago.

Download all attachments as: .zip

Change History (38)

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


3 years ago

#2 @antpb
3 years ago

  • Milestone changed from Awaiting Review to 5.9

Thanks for reporting this @ppetrov2c ! I'm leaving a comment to note that in 5.8.1 the count per page was increased to 80 so in the case of this report, you'll be wanting to check the 81st result to reproduce what's being reported in this ticket.

Marking 5.9 as we investigate. :)

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


3 years ago

#4 @antpb
3 years ago

  • Owner set to joedolson
  • Status changed from new to assigned
  • Summary changed from Attachment details pane navigation not working after opening 41st (or greater) item and refreshing the browser to Attachment details pane navigation not working after opening 81st (or greater) item and refreshing the browser

Noting with the changes that doubled the count loaded initially, the problem number will now be 81st or greater. :)

Last edited 3 years ago by antpb (previous) (diff)

#5 follow-up: @joedolson
3 years ago

This is directly caused by the fact that if you load more, then refresh the page, the collection is reset to the first 80 items, regardless of the additional items you've loaded. This doesn't stop a currently selected item from loading, but does break the item navigation, since the current item is effectively not in the collection.

There are a couple possible paths I can think of:

1) Implement a URL parameter that tracks the collection, so a refresh reloads everything previously loaded and the collection is inclusive. This would also be beneficial for introducing pagination.

2) Introduce an update to the previous item navigation so that it goes back to the last item in the current collection if the item is not itself in the current collection. Probably easier to implement, but I'm not sure it creates a particularly useful UI.

#6 @sabernhardt
3 years ago

  • Milestone changed from 5.9 to 6.0

Moving to 6.0 for now, though this might be appropriate in a minor release.

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


3 years ago

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


2 years ago

#9 @costdev
2 years ago

  • Keywords needs-patch dev-feedback added

Per the discussion in the bug scrub, I'm adding needs-patch and also dev-feedback so that the suggestions Joe outlined in this comment can get some input from others.

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


2 years ago

#11 @antpb
2 years ago

  • Milestone changed from 6.0 to 6.1

Considering the time remaining in milestone and the workload of folks that can bring this across the line, this should move to 6.1. Happy to move back if anyone is able to help out on it! :D

#12 in reply to: ↑ 5 @mdawaffe
21 months ago

Replying to joedolson:

There are a couple possible paths I can think of:

1) Implement a URL parameter that tracks the collection, so a refresh reloads everything previously loaded and the collection is inclusive. This would also be beneficial for introducing pagination.

2) Introduce an update to the previous item navigation so that it goes back to the last item in the current collection if the item is not itself in the current collection. Probably easier to implement, but I'm not sure it creates a particularly useful UI.

We could also decide that left/right navigation does not makes sense for a media item unless that media item is loaded by clicking through to it from the list view. That is, if you visit, for example, http://localhost:8889/wp-admin/upload.php?item=6 directly (or if you reload the page when on that URL), we could not show the left/right navigation buttons at all.

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


21 months ago

#14 @antpb
21 months ago

  • Milestone changed from 6.1 to 6.2

considering the new recommended path forward, this needs time to have an agreed and developed fix. Moving this to 6.2 to allow it the time needed.

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


19 months ago

#16 @antpb
19 months ago

  • Owner changed from joedolson to antpb

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


16 months ago

#18 @antpb
16 months ago

  • Milestone changed from 6.2 to 6.3

Joe and I have been discussing this one but think that it is likely best fit for 6.3 as any changes here have high risk of introducing issues.

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


12 months ago

#20 @costdev
12 months ago

Hi @antpb, is this likely to get a patch during the 6.3 cycle, or should we move it to Future Release until it has some more discussion and a patch?

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


11 months ago

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


11 months ago

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


11 months ago

#24 @antpb
11 months ago

  • Milestone changed from 6.3 to 6.4

Still having trouble solving this but it is related to some other issues I'm working on so I'll move this to 6.4 to keep it in view of Media.

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


9 months ago

#26 @nicolefurlan
8 months ago

@antpb just checking in on the status of this on since we're ~2 weeks from the 6.4 RC1. Do you think we'll be able to patch this ticket for 6.4?

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


8 months ago

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


8 months ago

#29 @nicolefurlan
8 months ago

  • Milestone changed from 6.4 to 6.5

@antpb pushing this one to 6.5 since it doesn't yet have a patch. Feel free to change it to Future Release if you think it needs more time/discussion.

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


4 months ago

@antpb
4 months ago

#31 @antpb
4 months ago

Added a patch here. When the library index exceeds the count that is displayed on refresh, it returns an index value of -1. If we click the first item in the media library, the index value correctly shows 0 which then gets -1 in the hasPrevious check and sets hasPrevious to false. In the case of the out of range index, the -1 gets -1 and becomes -2 so we can safely check for an initial value of -1 and deduce that it is outside the range of initial load and does indeed have previous items.

Trying to think of cases where -1 will incorrectly assume previous.

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


4 months ago

#33 @joedolson
3 months ago

So, this semi-works. It depends a bit on what our expectations are. This does re-enable the navigation; but it navigates back one place in the visible collection, which isn't the same as the collection that the item was opened from.

For example, if you click on the 85th item, it would normally go back to the 84th item. If you do the reproduction steps with the patch in place, you are able to navigate, but it goes back to the 79th item.

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


3 months ago

#35 @antpb
3 months ago

  • Milestone changed from 6.5 to 6.6

Going to move this to 6.6 as we have identified new challenges that need a bit more time to explore. I'm working on a refactor of my approach that tracks a count of triggers on the load more button so that we can initialize the collection with this insight. More to come!

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


2 months ago

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


31 hours ago

Note: See TracTickets for help on using tickets.