WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#25963 closed enhancement (fixed)

Appearance Themes: Backbone.Router should use pushState with ?query type urls

Reported by: matveb Owned by:
Milestone: 3.8 Priority: high
Severity: major Version: 3.8
Component: Customize Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

Make the Backbone routes pushSTate capable with ?theme=themename type urls instead of hashes. Same applies to search queries.

Attachments (14)

25963.diff (711 bytes) - added by adamsilverstein 6 years ago.
add pushState: true
25963.2.diff (796 bytes) - added by adamsilverstein 6 years ago.
correct duplicate overlay back button issue
25963.3.diff (4.5 KB) - added by adamsilverstein 6 years ago.
adds ?theme=name routing; also pushState: true
25963.4.diff (902 bytes) - added by adamsilverstein 6 years ago.
cleaner fix for duplicate overlay back button issue
25963.5.diff (6.3 KB) - added by adamsilverstein 6 years ago.
routing following revisions.js pattern
25963.6.diff (5.5 KB) - added by adamsilverstein 6 years ago.
simpler, remove unused routes, don't clutter history
25963.7.diff (5.2 KB) - added by adamsilverstein 6 years ago.
remove unused self = this
25963.8.diff (1.7 KB) - added by kovshenin 6 years ago.
25963.9.diff (6.8 KB) - added by adamsilverstein 6 years ago.
combines 7 & 8, fixes missing event in collapse call
25963.10.diff (6.8 KB) - added by adamsilverstein 6 years ago.
remove search debounce as per IRC discussion
25963.11.diff (7.4 KB) - added by nacin 6 years ago.
25963.regex-route-concept.diff (7.7 KB) - added by matveb 6 years ago.
25963.12.diff (3.8 KB) - added by adamsilverstein 6 years ago.
restore real JS routing
26543.13.patch (4.2 KB) - added by gcorne 6 years ago.

Download all attachments as: .zip

Change History (44)

#1 @adamsilverstein
6 years ago

Enabling pushState: true removes the hash marks from URLS so we get pretty URLS like themes.php/theme/twentyeleven. I think the only reason we were shooting for routes via query parameter like themes.php?theme=twentyeleven is to match the layout of the revisions URLS.

According to the Backbone forums ( see here, here and here ) query args are now completely ignored by Backbone.Router, and the authors are of the opinion that client side code shouldn't use query parameters at all.

I tried matching the setup we used in revisions to map the route, but when using '?' my route is never fired. There are workarounds, and this plugin adds query args. I think this deserves further discussion: do we really need query arg route mapping? I think revisions kept the ?revision=ID mapping originally to match the previous system's URLS - I don't think we have that need here and I'm inclined to use regular 'path mapping'.

@adamsilverstein
6 years ago

add pushState: true

#2 @matveb
6 years ago

I had it working with pushState in the form of themes.php/theme/twentyeleven but it felt a bit awkward so went with hashes instead.

#3 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 3.8

#4 @carldanley
6 years ago

  • Cc carldanley@… added

#5 follow-up: @carldanley
6 years ago

Let me know if you need something put together for this; would love to help!

#6 in reply to: ↑ 5 @adamsilverstein
6 years ago

Replying to carldanley:

Let me know if you need something put together for this; would love to help!

Your expertise & input appreciated! the original idea was to use query string urls like we currently use in revisions. I wasn't able to get it working without some kludgy code. The url would look like:

wp-admin/themes.php?theme=[theme_name]

I believe the impetus was to match revisions and possibly provide a no-js fallback (since the theme name is passed to PHP this way). However, Backbone seems to be moving away from allowing/supporting query string routing. if we just enable pushState, we get URLS like this:

wp-admin/themes.php/theme/[theme_name]

which is OK, perhaps a bit prettier would be something like

wp-admin/themes/[theme_name] which should require adding some logic to wp-admin/index.php

you will also note a search route in the code, so far we've only talked about the theme route.

#7 follow-up: @ocean90
6 years ago

Currently there is an issue with the browser history and closing the modal.

  • Go to wp-admin/themes.php
  • Open info modal of Twenty Eleven
  • Navigate to the next theme, Twenty Fourteen for me
  • Click browsers back button
  • You should see Twenty Eleven again
  • Try to close the modal
  • Modal isn't closed, it shows Twenty Fourteen again
  • Try to close the modal again
  • Now it's closed

So if you are using browsers back button x-times you have to click the modal close icon x-times too.

#8 @rmccue
6 years ago

  • Severity changed from normal to minor

#9 @nacin
6 years ago

  • Priority changed from normal to high
  • Severity changed from minor to major

We need to figure this out. I see two problems:

  • The bug mentioned by ocean90
  • The lack of query string URLs

I actually find the lack of query string URLs to be a pretty major issue. Not as much for ?theme= but for ?s=, which is broken. :-( Otherwise there is no good way to "link" someone to a particular theme.

#10 @carldanley
6 years ago

I didn't have a ton of time but hopefully this will help somehow. I put together a function for you that parses the GET params of a URL.

https://gist.github.com/carldanley/e3e1f609091f67356f59

You can play around with this function here: http://jsfiddle.net/zwb8E/ .

  • If the URL contains multiple ?'s, the last ?'s parameters are used.
  • If the URL contains an empty GET param, it will have a value of undefined.

My idea is that this can be wrapped into something, somewhere to help you support this. Let me know if you have any questions.

#11 follow-up: @ocean90
6 years ago

Related router part from revisions: trunk/src/wp-admin/js/revisions.js@26162#L1051

#12 in reply to: ↑ 11 @adamsilverstein
6 years ago

Replying to ocean90:

Related router part from revisions: trunk/src/wp-admin/js/revisions.js@26162#L1051

when i tried this approach with the current code it failed to call the routes at all. i will take another look!

@adamsilverstein
6 years ago

correct duplicate overlay back button issue

#13 in reply to: ↑ 7 @adamsilverstein
6 years ago

Replying to ocean90:

Currently there is an issue with the browser history and closing the modal.

  • Go to wp-admin/themes.php
  • Open info modal of Twenty Eleven
  • Navigate to the next theme, Twenty Fourteen for me
  • Click browsers back button
  • You should see Twenty Eleven again
  • Try to close the modal
  • Modal isn't closed, it shows Twenty Fourteen again
  • Try to close the modal again
  • Now it's closed

So if you are using browsers back button x-times you have to click the modal close icon x-times too.

25963.2.diff corrects issue with back button - multiple overlays were being created, this change prevents that. working on pushState next. please review.

@adamsilverstein
6 years ago

adds ?theme=name routing; also pushState: true

#14 @adamsilverstein
6 years ago

In 25963.3.diff:

  • added ?theme=name routing; also pushState: true
  • search routing next!

#15 @carldanley
6 years ago

Looks good. L708 needs indent though. Also, maybe some function commenting to keep things organized. Good stuff adamsilverstein :)

@adamsilverstein
6 years ago

cleaner fix for duplicate overlay back button issue

#16 @adamsilverstein
6 years ago

25963.4..diff​ is a cleaner fix for the back button issue mentioned above by ocean90.

@adamsilverstein
6 years ago

routing following revisions.js pattern

#17 @adamsilverstein
6 years ago

in 25963.5.diff

  • use a routing methodology similar to revisions - php checks for initially passed variables and JS sets state appropriately
  • back button no longer works (although reloading or bookmarking works)
  • testing revisions again in trunk and back to 3.6.1 I'm seeing that the back button also doesn't work there so behavior ok?

#18 @adamsilverstein
6 years ago

25963.6.diff:

  • removes unused routes
  • don't clutter history - navigate with replace: true

@adamsilverstein
6 years ago

simpler, remove unused routes, don't clutter history

@adamsilverstein
6 years ago

remove unused self = this

@kovshenin
6 years ago

#19 @kovshenin
6 years ago

25963.8.diff deals with the keyboard navigation lag.

The issue occurs inside themes.view.Details.navigation() which gets fired during .render() meaning a new keyup event is attached to body every time the details view is rendered. After a while this becomes very laggy. The patch moves the event binding to themes.view.Themes.initialize() instead, and calls the overlay methods directly, which then trigger the appropriate events.

@adamsilverstein
6 years ago

combines 7 & 8, fixes missing event in collapse call

#20 @adamsilverstein
6 years ago

looks good. the only issue i found was firefox throwing an undefined console error on the collapse call.

25963.9.diff​ corrects that by explicitly passing the event to collapse and combines the changes from 25963.7.diff​ and 25963.8.diff​

#21 @nacin
6 years ago

  • Keywords has-patch added

How's it looking? This about ready? :-) What still needs to be done?

@adamsilverstein
6 years ago

remove search debounce as per IRC discussion

#22 @adamsilverstein
6 years ago

works well in my testing. would like to try more testing in RTL/various browsers

@nacin
6 years ago

#23 @nacin
6 years ago

This works beautifully in my testing. 25963.11.diff updates it to trunk. It also removes extraRoutes.

#24 @nacin
6 years ago

  • Keywords commit added

#25 @ryan
6 years ago

Make the Backbone routes pushSTate capable with ?theme=themename type urls instead of hashes. Same applies to search queries.
Props adamsilverstein, nacin

[26767]

#26 @ryan
6 years ago

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

#27 @nacin
6 years ago

In 26778:

Fix JSHint errors and remove unreachable code. props adamsilverstein. see #25963.

#28 follow-up: @matveb
6 years ago

I have been working on a possible solution that will allow us to use routing instead of passing the url params via localize script. We can parse the url parameters from Backbone.history and then trigger proper route:theme events which we hook to the same way we were doing before.

I have it working but things got out of sync with the latest commit here. This is what I propose, though, what do you think?

https://cloudup.com/cPHH02p4W9s

(The RegEx can probably be tweaked to be more robust.)

Last edited 6 years ago by matveb (previous) (diff)

@adamsilverstein
6 years ago

restore real JS routing

#29 in reply to: ↑ 28 @adamsilverstein
6 years ago

Replying to matveb:

I have been working on a possible solution that will allow us to use routing instead of passing the url params via localize script. We can parse the url parameters from Backbone.history and then trigger proper route:theme events which we hook to the same way we were doing before.

I have it working but things got out of sync with the latest commit here. This is what I propose, though, what do you think?

https://cloudup.com/cPHH02p4W9s

(The RegEx can probably be tweaked to be more robust.)

Excellent - I really prefer this all JavaScript approach to the routing - it seems to work well and is pretty straightforward (other than the extracting the args from Backbone.history.location.search). It deserves more testing, hopefully this patch helps with that.

25963.12.diff introduces your approach against trunk and removes the wp_localize_script pass of theme and search vars.

we should add a ?s=themename url for backward compatibility with existing links (or how about just keeping ?s=themename as the search string?)

#30 @matveb
6 years ago

Great! I started a more general purpose ticket for solving this at a wp-backbone level instead, so also revisions (and anything new) benefits from it. See #26543.

@gcorne
6 years ago

Note: See TracTickets for help on using tickets.