Making WordPress.org

Opened 4 years ago

Last modified 7 months ago

#4814 reopened defect (bug)

Pagination broken on themes

Reported by: jonoaldersonwp's profile jonoaldersonwp Owned by:
Milestone: Priority: normal
Component: Theme Directory Keywords: seo
Cc:

Description

Attempting to navigate to paginated states of theme results (e.g., https://wordpress.org/themes/tags/right-sidebar/, and https://wordpress.org/themes/browse/popular/page/3/) results in a 302 redirect to the series root.

This should instead return the correct paginated state.

Attachments (2)

4814.diff (10.9 KB) - added by dufresnesteven 4 years ago.
Screen Shot 2020-04-02 at 12.57.13 PM.png (1.4 MB) - added by dufresnesteven 4 years ago.

Download all attachments as: .zip

Change History (31)

#1 @Otto42
4 years ago

I'm somewhat confused, as the themes directory has no pagination. It's an infinite scroller.

#2 @jonoaldersonwp
4 years ago

Disable JavaScript / look at the source code.

#3 @dd32
4 years ago

  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #4092.

#4 @jonoaldersonwp
4 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

This did work previously, and has since broken. It's currently a critical issue, and we can't wait on an ethereal timeline for a backbone/JS solution.

#5 follow-up: @dd32
4 years ago

  • Resolution set to duplicate
  • Status changed from reopened to closed

See [8126] on #4092 - Intentional

#6 @jonoaldersonwp
4 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

I'm not sure what I'm looking at in that changelog, but intentional or otherwise, this needs fixing.

It was previously the case that it was possible to request and return paginated states of these archives. That's now not the case. That functionality needs to be restored.

#7 in reply to: ↑ 5 @dd32
4 years ago

Replying to dd32:

See [8126] on #4092 - Intentional

Sorry, that was [9064] via #4613

This seems fine as-is.

#8 @dd32
4 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

Closing as wontfix instead then.

We don't support pagination on the current-generation theme directory, and no links should exist to it.

We may add pagination in the future, when/if the theme directory gets re-engineered.

#9 follow-up: @jonoaldersonwp
4 years ago

Ah, so these should 404, not 302.

#10 in reply to: ↑ 9 @dd32
4 years ago

Replying to jonoaldersonwp:

Ah, so these should 404, not 302.

In an ideal universe, but if links exist in the wild as you suggest, directing to the index is the best user experience here.

#11 follow-up: @jonoaldersonwp
4 years ago

Gah. Yes, but this is really bad. Can we up the priority on the JS fix stuff? OR, support pagination here?

#12 in reply to: ↑ 11 @dd32
4 years ago

Replying to jonoaldersonwp:

Gah. Yes, but this is really bad. Can we up the priority on the JS fix stuff? OR, support pagination here?

Both are super-low priority, and realistically will never be supported unless the directory has a new UI and backend added.

#13 @jonoaldersonwp
4 years ago

  • Priority changed from highest omg bbq to high
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Revisiting this one, as it's an absolutely crippling SEO, UI and accessibility nightmare.

TL;DR, the themes directory uses JavaScript to provide an 'infinite scroll' mechanism, but conventional pagination (whilst exposed) is broken.

With JS disabled, the themes template(s) contains/reveals a pagination component (which links to a series of /page/n/ derivatives of the current query).

However, requesting paginated URLs triggers a 302 redirect to the series root.

In short, we _have_ to support pagination on themes, regardless of the complexity or resource cost. This is non-negotiable, as theme discovery, crawling and indexing is currently severely impacted.

Requests to https://wordpress.org/themes/page/4/ (and similarly, filtered flavours, like https://wordpress.org/themes/browse/new/page/4/) must return the (server-side) appropriately paged results for the query in question.

This may require some additional adjustment of the JS 'on top' of the server-side response.

This ticket was mentioned in Slack in #meta by jonoaldersonwp. View the logs.


4 years ago

#15 @jonoaldersonwp
4 years ago

  • Priority changed from high to highest omg bbq

Resurrecting this, as it's an absolute nightmare from a marketing/SEO/brand/accessibility perspective.

This ticket was mentioned in Slack in #meta by joyously. View the logs.


4 years ago

@dufresnesteven
4 years ago

#17 @dufresnesteven
4 years ago

I have uploaded a diff that will add paging to the themes page: 4814.diff

What it does:
If a user requests a specific page themes/page/2/ we will request that page and add an alert (as seen in the screenshot) informing the user that they are viewing a subset of themes. They can click the View All button which will reload the page.

What it doesn't do:
Paging only works on the categories. It does not add support for paging on tags or search results.
It also doesn't change anything if js is disabled (meaning nothing is rendered server side).



It's worth noting that we don't give user's an entry path into the paging feature. It's more for SEO purposes.

Last edited 4 years ago by dufresnesteven (previous) (diff)

#18 follow-up: @jonoaldersonwp
4 years ago

Can we replace "You are viewing a subset" with a conventional pagination widget?

And, if this isn't server-side, it won't solve the problem(s). :(

Also, we shouldn't be outputting warnings/notices in the rendered HTML, then hiding them with CSS/JS. That's going to cause confusion and issues for search engines and for users.

More broadly, if we're just doing stuff "For SEO purposes", we're probably doing it wrong. This really needs fixing properly, I'm afraid, for users as much as for search engines.

Last edited 4 years ago by jonoaldersonwp (previous) (diff)

#19 in reply to: ↑ 18 @dufresnesteven
4 years ago

Replying to jonoaldersonwp:

More broadly, if we're just doing stuff "For SEO purposes", we're probably doing it wrong. This really needs fixing properly, I'm afraid, for users as much as for search engines.

I think you're hanging on a bit too tightly here and looking at this as an all or nothing approach. I think that is the wrong approach, especially when we are looking at complicated changes, identified in the conversations above (and associated tickets).

As you mention:

In short, we _have_ to support pagination on themes, regardless of the complexity or resource cost. This is non-negotiable, as theme discovery, crawling and indexing is currently severely impacted.

...

And, if this isn't server-side, it won't solve the problem(s). :(

Are we convinced that Google's indexer will not pick up the changes in the second wave of indexing?

#20 @jonoaldersonwp
4 years ago

Nope, Google's JS support isn't nearly as good as they say it is. We're sending mountains of conflicting and erroneous signals underneath/around the rendered content. There are also about a billion other issues throughout the site(s) which cause crawling, indexing and budgeting problems, which makes it even less likely that just hoping that it'll work correctly will end well for us.

This needs to be fixed, and correct, and consistent between server-side/client-side. That this is a complicated change doesn't mean we should compromise by adding more JS band-aids and making it messier. I'd much rather we just ripped it out, implemented a conventional, sensible server-side response (and potentially added in some client-side infinite loading if we feel that's valuable), than spend months and years iterating with minor tweaks which don't address the problem.

#21 @dd32
4 years ago

Can you outline an implementation that, in your opinion, will work with infinite scroll then? I really can't tell what you expect the answer to be, if not 4814.diff.

The core UX of the infinite scroll is not going to be removed, the JS is not going to be removed, if anything, the server-side rendering is the more likely part to get removed and we've been adding back bits and pieces of it constantly in response to tickets exactly like this.

#22 @dd32
4 years ago

Would 4814.diff plus the no-js pagination widget below the infinite scrolling list work? Most users wouldn't ever see the pagination selector.. google probably wouldn't either..

#23 @jonoaldersonwp
4 years ago

We can't move (further) away from server-side rendering unless we have a pre-rendering/isomorphic solution in place. Given that's unlikely to happen (and that it'd be an equivalent amount of effort just to do this right), then we need to make this crawlable, indexable and accessible. Relying on fudges and no-js hacks isn't a viable option. We need the behaviour to be correct and optimal without JS, and then to layer in progressive enhancement. Anything else will (continue to) damage our discoverability, market share, reputation, etc.

Anecdotally, we're already seeing examples where the current approach causes unforeseen issues because it's over-engineered. E.g., our recent addition of lazy loading for images was ineffectual, because we load theme tiles in an unnecessarily complex manner. We're likely to unearth hundreds of such issues as we try and get best practices in place for SEO and accessibility.

We're making the same tedious mistakes which all large organizations and corporations make when they JavaScript because they can, not because they should. I'd hope that we could dogfood better.

Our simplest option is to use a server-side solution which behaves like a conventional WordPress archive (posts per page, pagination, adaptive metadata, etc). On top of that, we can add a JS solution for lazy-loading additional pages as the user scrolls.

With regards to infinite scrolling, we'd need to:

  • Use the intersectionObserver API to determine when to lazily load in the next page's results (NOT scroll detection / viewport positioning, etc; though polyfill as necessary
  • Use the history API to update the URL (and meta) to /page/n/ as the browser reaches/triggers those boundaries (in both directions).
Last edited 4 years ago by jonoaldersonwp (previous) (diff)

#24 @dd32
4 years ago

Thank you @jonoaldersonwp.

Based on that, there's two observations I can make

Firstly, Server-side rendering is incompatible with Backbone JS for the purposes of this, as while we emulate an isomorphic situation, the JS will always override it and re-render it, potentially with different data.

Secondly, We're better off putting this on the backburner for the foreseeable future, until a time where we replace the Theme Directory with a new front-end based off more modern platform/JS.

The effort required here to make this work in the way that's required is not small and effectively requires rewriting the entire front-end, something that's going to be better off done with #45 and #215 in mind.
Priority is currently focused on the other directories, but with the intention that they'll make a better foundation for bringing the Theme Directory over to a newer codebase in the future.

Getting Sitemaps (#5022) working will hopefully fill the intermediate gap.

#25 @jonoaldersonwp
4 years ago

Fair enough!

If we can prioritise #5022 (which will likely come with some other headaches and sub-tickets), then that should buy us some time!

#26 @dd32
4 years ago

#4092 was marked as a duplicate.

#27 @dd32
4 years ago

In 9860:

Theme Directory: Remove the no-js pagination links until supported.

See #4814.

#28 @Otto42
7 months ago

  • Priority changed from highest omg bbq to normal

#29 @jonoaldersonwp
7 months ago

How come we're downgrading the prio on this? It's still on fire ;)

Note: See TracTickets for help on using tickets.