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

Query loop with 2 columns and enhanced pagination causes last page to only have 1 column #55230

Closed
beckej13820 opened this issue Oct 10, 2023 · 18 comments
Labels
[Block] Query Loop Affects the Query Loop Block [Feature] Interactivity API API to add frontend interactivity to blocks. [Type] Bug An existing feature does not function as intended

Comments

@beckej13820
Copy link

Description

This bug was seen on a website running WordPress 6.3.1 with the latest version of the Gutenberg plugin 16.7.1. The theme used on the site is the core theme Twenty twenty-two.

On both the homepage and the archive page, I have a query loop set up that is set with two columns and the setting for enhanced pagination is turned on. In both locations, when I go to the second page, I only see 1 column.

I have created video where I demonstrate the bug, and then go through my key settings:
https://www.youtube.com/watch?v=TrNdwRrcvTo

0-1:00 I introduce the problem and demonstrate it.
1:00-3:00 I show my query loop and blocks inside in detail. Notable is that this site is using custom taxonomies created through TaxoPress. Using core WordPress blocks to display those custom taxonomies. Query loop is set to inherit settings.
3:00-4:00 I turn off enhanced pagination and show that the query loop works as expected when enhanced pagination is off.
4:30-end I show you around my wordpress site, show you all plugins, show version numbers.

Not shown in video:
This site is using the theme Twenty Twenty Two.
This bug still shows up when the Search and Filter Pro plugin is disabled.

Step-by-step reproduction instructions

I'm having a hard time replicating this bug.

In my case, it happens every time I have 2 columns and enhanced pagination on, and a large enough pool of posts that it needs two pages. The bug only appears on page 2, not on page 1.

Screenshots, screen recording, code snippet

No response

Environment info

This bug was seen on a website running WordPress 6.3.1 with the latest version of the Gutenberg plugin 16.7.1. The theme used on the site is the core theme Twenty twenty-two.

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@beckej13820
Copy link
Author

@annezazu Tagging Anne because she asked me to.

@jordesign jordesign added [Type] Bug An existing feature does not function as intended Needs Testing Needs further testing to be confirmed. [Block] Query Loop Affects the Query Loop Block labels Oct 11, 2023
@annezazu
Copy link
Contributor

Thanks for opening this and tagging me! Just tried to replicate this and I cannot.

pagination.mov

This is using 6.4-beta3 and GB 16.8. I'm not sure what's going on here so leaving open for others.

@beckej13820
Copy link
Author

I copied the content from this site into a fresh site, disabled all other plugins, and deleted my existing query loops. I have confirmed. Here were the steps I used.

  1. Have a test site with multiple posts. I have 19 posts on this site.
  2. Enable Twenty Twenty two.
  3. Start a new query loop on the frontpage. I created my own, started from the template with image, date, and title
  4. Use the side by side query view instead of the list view. Inherit the query from the template, turn on enhanced pagination.
  5. Set the pagination on the low end in the site editor, on my demo I used 5 per page. With 19 posts, this means that I will have 3 full pages of posts, plus on the 4th and final page, I should have 4 posts.

Result: The first 3 pages in the query loop work as expected, shown side by side. The final page with 4 posts reverts to a single column.

@beckej13820
Copy link
Author

beckej13820 commented Oct 12, 2023

I'm still playing with this. I tried a few different settings.

  • I could still replicate with lower numbers.
  • If the number of posts divided evenly into the number of posts displayed, the final page of the query loop works as expected. If the "remainder" is not equal to the max number of posts per page, that's when it appears in a single column instead of two columns.

I replicated on a site with 20 posts. I set the site editor to show pages of 6, and set my query loop to inherit. The result is 3 full pages, and on the forth page, the 2 remainders are shown as full width instead of being side by site.

I'll leave the site up for others to look: https://demo.ed-beck.com/showcase

@annezazu in your video- note that the final page in your demo is full width. It didn't jump out at you, because you were dividing in groups of 3 and it would have been the sole remainder, but I bet if you added another post to that same site, the final page of the query would be in a single column, even if there were two of them.

@annezazu annezazu changed the title Query loop with 2 columns and enhanced pagination causes page 2 to only have 1 column Query loop with 2 columns and enhanced pagination causes last page to only have 1 column Oct 12, 2023
@annezazu annezazu added the [Feature] Interactivity API API to add frontend interactivity to blocks. label Oct 12, 2023
@annezazu
Copy link
Contributor

@luisherranz noting this for you! It seems this happens to the very last page in the pagination.

@annezazu annezazu removed the Needs Testing Needs further testing to be confirmed. label Oct 12, 2023
@annezazu annezazu moved this to Needs Dev / Todo in WordPress 6.4 Editor Tasks Oct 12, 2023
@beckej13820
Copy link
Author

Now that we have clearer directions to replicate, I've replicated in theme twenty twenty three as well. The error looks a little different, but I suspect the cause is the same. This is a query loop with 9 posts, displays 5 at a time in 2023:

In 2023, the final page displays in a single column just like 2022. Unlike 2022, the final page retains the expected width.

page 1 of query
Screenshot 2023-10-11 at 8 17 38 PM

page 2 of query
Screenshot 2023-10-11 at 8 17 52 PM

@luisherranz
Copy link
Member

Interesting. In this enhanced pagination implementation, we are not updating the CSS, only the HTML. So my first impression is that the CSS that is generated for the last page of the Query Loop is different than the rest of the pages.

We'll investigate the issue. Thanks for the very detailed report and the video 🙂

@luisherranz
Copy link
Member

This is the CSS diff between the two pages.

Screenshot 2023-10-12 at 10 44 12

We'll investigate where this is generated and why it needs to be different to see if we can refactor it to fix the problem.

@tellthemachines
Copy link
Contributor

tellthemachines commented Oct 13, 2023

@luisherranz when the HTML is updated, would it be possible to preserve all the classnames on the Post Template block container, including the wp-container-x? Currently when switching pages with the enhanced pagination, the number on that classname changes, so if on page 1 it's wp-container-15, on page 2 it will be wp-container-14 and so on.

The problem is that layout styles are attached to these classnames, and both classnames and styles are generated on the server side. So if we could preserve the same classname throughout the pages, the styles will always apply.

Edit: I notice the same thing happens to the Query Pagination block if it has any non-default layout styles (try setting justification to center for example). I think the problem is that when we load a new page from the server, those classname numbers aren't always the same, but that doesn't matter because the correct styles will be output together with the classname, whereas when we fetch the markup with JS we're not getting the styles.

I'm not sure what the best solution is tbh - an alternative would be to refresh the styles with JS too.

@luisherranz
Copy link
Member

Thanks, @tellthemachines. I only took a quick look to see where those class names are generated, which seems to be in the logic of the layout block supports.

I still need to debug it to understand why this is only broken on the last page because as far as I knew, the number generated by wp_unique_id should always be incremental, which means that as long as blocks render in the same order, everything should receive the same number (class name) even between different pages.

I'll debug it as soon as possible, but it might take me a few days as I'm a bit swamped at this moment.

@bph bph moved this from Needs Dev / Todo to Punted to 6.5 in WordPress 6.4 Editor Tasks Oct 13, 2023
@bph bph moved this from Punted to 6.5 to Needs Dev / Todo in WordPress 6.4 Editor Tasks Oct 13, 2023
@bph bph moved this from Needs Dev / Todo to Punted to 6.5 in WordPress 6.4 Editor Tasks Oct 13, 2023
@tellthemachines
Copy link
Contributor

I still need to debug it to understand why this is only broken on the last page because as far as I knew, the number generated by wp_unique_id should always be incremental

It's not only broken on the last page - I can reproduce the issue on all pages after the first, on both the Post Template and the Pagination blocks 😬 and yes, it's unexpected that these numbers vary from page to page. I might be able to look into it sometime this week!

@annezazu
Copy link
Contributor

I am going to add this to 6.4.x rather than 6.5 as this would be good to fix for a point release.

@annezazu annezazu moved this from Punted to 6.5 to Punted to 6.4.1 in WordPress 6.4 Editor Tasks Oct 16, 2023
@tellthemachines
Copy link
Contributor

Ok I think I've worked out what the problem is! wp_unique_id is incrementing every time it runs, and it's not only being used in layout. So e.g. if there's a duotone image on the page, or a gallery, or anything else that has a wp_unique_id, the number changes.

For Post Template specifically this is problematic because its content varies depending on the post: if featured images use duotone but not every post has a featured image, then the wp_unique_id assigned to Post Template is very likely going to be different across pages, especially because duotone seems to run before layout.

Not quite sure how best to fix this yet; I'll have a think about it and try to get a PR going in the next few days.

@luisherranz
Copy link
Member

Thanks for looking into this, @tellthemachines ❤️

So what about mofifying the wp_unique_id function so instead of using a global number, uses a number per prefix? After all, ids contain the prefix so they should never collide.

Replace this:

function wp_unique_id( $prefix = '' ) {
	static $id_counter = 0;
	return $prefix . (string) ++$id_counter;
}

wp_unique_id( 'x' ) // x1
wp_unique_id( 'x' ) // x2
wp_unique_id( 'y' ) // y3

With this:

function wp_unique_id( $prefix = '' ) {
    static $id_counters = array();

    if ( ! array_key_exists( $prefix, $id_counters ) ) {
        $id_counters[ $prefix ] = 0;
    }

    return $prefix . (string) ++$id_counters[$prefix];
}

wp_unique_id( 'x' ) // x1
wp_unique_id( 'x' ) // x2
wp_unique_id( 'y' ) // y1

I guess it would be a safe change, wouldn't it?

@tellthemachines
Copy link
Contributor

I guess it would be a safe change, wouldn't it?

Hard to say, because that function has been in core for a while and is being used by all sorts of plugins 😅 so if anyone is depending on its current behaviour of incrementing across prefixes, that would be a breaking change.

A possible alternative would be to create a new wp_prefix_specific_unique_id function (preferably with a better name 😂 ) that we can use in layout and wherever else the wp_unique_id behaviour may cause problems.

@tellthemachines
Copy link
Contributor

Thinking a bit more about this, changing the way the unique id is generated won't fix the underlying issue, because if there is any variation in the content of the dynamically loaded pages things can still break. For instance, try the following:

  1. Set a duotone filter for the Featured Image block;
  2. Adjust max post per page number to 4;
  3. Add/remove featured images from posts such that the first page only has one post with a featured image, but the second page has 3 or 4;
  4. Navigate to the second page and verify that only the first image has the correct duotone filter.

The problem is that only the styles from the first page are loading. I think we need to work out a way of fetching the styles for the subsequent pages as well as the markup.

@luisherranz
Copy link
Member

Thanks for the exploration, @tellthemachines.

I've opened a couple of PRs to address both the layout and duotone class name stability across paginations:

Let's see if it's safe to land those fixes.

@luisherranz
Copy link
Member

Both PRs are now merged, so this issue should be solved.

Closing now as completed, but feel free to repone if more issues appear.

@github-project-automation github-project-automation bot moved this from Punted to 6.4.1 to Done in WordPress 6.4 Editor Tasks Oct 20, 2023
@annezazu annezazu moved this from Done to Punted to 6.4.1 in WordPress 6.4 Editor Tasks Oct 20, 2023
@mikachan mikachan added Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release and removed Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release labels Nov 3, 2023
@mikachan mikachan moved this from Punted to 6.4.1 to Done in WordPress 6.4 Editor Tasks Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Query Loop Affects the Query Loop Block [Feature] Interactivity API API to add frontend interactivity to blocks. [Type] Bug An existing feature does not function as intended
Projects
No open projects
Status: Done
Development

No branches or pull requests

6 participants