Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#20507 closed defect (bug) (fixed)

3.4 Preview/Customize page "Return to Manage Themes" link doesn't work as expected

Reported by: tomauger's profile TomAuger Owned by: koopersmith's profile koopersmith
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.4
Component: Customize Keywords:
Focuses: Cc:

Description

The link that's supposed to take you back to the Manage Themes page doesn't work as expected (browser: Google Chrome).

If you are customizing a theme and make any changes, the "Return" link at the top of the sidebar doesn't take you back to the Manage Themes page.

For each change that you make (ie: when editing one of the fields), you have to click that many times on the Return link before you get taken back to the main page. It's as if each "autosave" onChange of each input field gets stored in the browser's history and you have to go back that many times before you go back to the page you're expecting.

And it doesn't work like an incremental Undo either - clicking the link even once undoes all the changes you made (unless you Save). But you still have to click the link that many times before it actually takes you back.

While I'm on the topic - I wonder whether it's a mistake to make the "Save" button take you out of the Customize mode? As an end user, it surprised me that it didn't just save, but leave me there.

Attachments (8)

20507.diff (5.5 KB) - added by koopersmith 12 years ago.
20507.2.diff (5.7 KB) - added by ryan 12 years ago.
Handle $wp_scripts not being set
20507.3.diff (823 bytes) - added by ryan 12 years ago.
Allow origin
20507.4.diff (1.9 KB) - added by mdawaffe 12 years ago.
20507.5.diff (2.0 KB) - added by ryan 12 years ago.
Origin functions
20507.6.diff (1.2 KB) - added by ryan 12 years ago.
Just filters for now.
20507.7.diff (2.2 KB) - added by ryan 12 years ago.
With a send origin headers func.
20507.8.diff (3.1 KB) - added by ryan 12 years ago.
Cleanup, phpdoc

Download all attachments as: .zip

Change History (44)

#1 @SergeyBiryukov
12 years ago

  • Milestone changed from Awaiting Review to 3.4

Tested in Firefox 11, Chrome 18, IE 8, Opera 11.62, Safari 5.1.5. Reproduced in Chrome and Safari.

#2 @ryan
12 years ago

  • Component changed from Themes to Customizer

#4 follow-up: @koopersmith
12 years ago

  • Owner set to koopersmith
  • Status changed from new to accepted

Thanks, Helen. To fix this, we should declare the iframe's src attribute in the html when we create the iframe (instead of setting it afterwards).

#5 in reply to: ↑ 4 @koopersmith
12 years ago

  • Severity changed from normal to major

Replying to koopersmith:

To fix this, we should declare the iframe's src attribute in the html when we create the iframe (instead of setting it afterwards).

After looking through the code again, this isn't a possible solution because we need to send the control data to the iframe via POST (which is currently being accomplished by setting the URL as the action of the form and the iframe as the target).

Instead, I'm looking into whipping up a ajax-based solution that will inject the proper html into the iframe.

#6 @nacin
12 years ago

  • Severity changed from major to blocker

Per koopersmith, this should be considered a blocker for beta 4.

#7 follow-up: @TomAuger
12 years ago

Guys, as a workaround until you implement the ajax-based solution, would you consider using JavaScript and the browser history to go back to the correct page?

#8 in reply to: ↑ 7 @koopersmith
12 years ago

Replying to TomAuger:

Guys, as a workaround until you implement the ajax-based solution, would you consider using JavaScript and the browser history to go back to the correct page?

I don't think this is possible; if the browser records state as a full refresh, history.go( -1 * numberOfChanges ) will, in turn, trigger a full refresh. Plus, this could cause additional edge cases in non-history compliant browsers.

I'll land the ajax fix shortly.

#9 @koopersmith
12 years ago

In [20645]:

Theme Customizer: Migrate to an ajax-based solution for refreshing the preview and saving. see #20507, #19910.

  • Use ajax-based saving, add saving indicator.
  • Use ajax-based refreshing instead of form targets.
  • Instead of using hidden inputs with prefixed names to track the canonical data, use the values stored in wp.customize. Encode the values as JSON before sending to avoid bugs with ids that contain square brackets (PHP mangles POST values with nested brackets).
  • Use wp.customize.Previewer solely for the purpose of previewing; move the postMessage connection with the parent frame and other unrelated code snippets into the 'ready' handler.

#10 @koopersmith
12 years ago

  • Severity changed from blocker to normal

Please test the changes in [20645] across browsers — I'd like to identify and fix any cross-browser issues as quickly as possible. I'm leaving it open for this reason.

This ticket is no longer a blocker for beta 4. Go team.

#11 @ryan
12 years ago

XMLHttpRequest cannot load http://foo.wordpress.org/. Origin https://foo.wordpress.org is not allowed by Access-Control-Allow-Origin.

We need to figure out origin access, especially for multisite installs that use a domain mapping plugin. Thoughts?

#12 @ryan
12 years ago

Tentative plan after talking to koopersmith and nacin:

  • Serve both the customizer and the preview from the front so they both have the same origin.
  • If the admin is forced SSL, serve the customizer and preview over SSL as well.
  • Save via ajax when the origin and the destination match, as is currently done.
  • Save via POST and then redirect back to customizer when the origin and dest do not match. This avoids origin issues and allows referencing the admin auth cookies rather than just the logged in cookie.
Last edited 12 years ago by ryan (previous) (diff)

@koopersmith
12 years ago

#13 @koopersmith
12 years ago

Attached patch successfully serves the customizer and preview from the front end.

Forced SSL trickery and save via POST are *not* included in this patch.

@ryan
12 years ago

Handle $wp_scripts not being set

#14 @ryan
12 years ago

PHP Notice:  Trying to get property of non-object in /wp-includes/customize-controls.php on line 17

Evidently my test sites don't call any of the script loader functions that setup $wp_scripts before wp_loaded is fire. Patch handles this.

#15 follow-up: @ocean90
12 years ago

  • Cc ocean90 added

@ryan
12 years ago

Allow origin

#16 in reply to: ↑ 15 @mdawaffe
12 years ago

Since HTTP and HTTPS aren't the same origin, we also need to explicitly send credentials in the AJAX request and allow credentials in the CORS response.

Attached in 20507.4.diff

Also note that in http://www.w3.org/TR/cors/#resource-requests, the spec states we should not set any Access-Control headers in the response if the Origin in the request does not match our list of acceptable origins. I think the @header() calls should be moved into the if block, and the else block should be removed. Not patched.

@mdawaffe
12 years ago

#17 @koopersmith
12 years ago

In [20741]:

Theme Customizer: Add cross-domain handling for when the admin and front-end are different origins. Handles both ajax and postMessage calls. props rboren, mdawaffe, nacin. see #20507, #19910.

#18 @ryan
12 years ago

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

@ryan
12 years ago

Origin functions

@ryan
12 years ago

Just filters for now.

@ryan
12 years ago

With a send origin headers func.

@ryan
12 years ago

Cleanup, phpdoc

#19 @ryan
12 years ago

In [20793]:

Don't force frontend scheme to match backend. This fails if the frontend doesn't have proper SSL certs. Access-Control-Allow-Origin handles this without the need to make the schemes match. see #20507

#20 @johnbillion
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This is still broken. Steps to reproduce:

  1. Visit the theme customizer and change some settings if you feel like it.
  2. Click a link to another page on the site.
  3. Click 'Return to Manage Themes'.

You'll simply be taken back a page in your browser's history instead of being taken back to the Themes screen.

Last edited 12 years ago by johnbillion (previous) (diff)

#21 @ocean90
12 years ago

Which browser? Couldn't reproduce it with Chrome dev and Firefox 12 on OS X.

#22 @johnbillion
12 years ago

Ah-ha, this is happening when you click a link which results in a server side redirect. An example of this is clicking the Random Member or Random Group link in the BuddyPress admin bar menu. After the redirect has happened, the theme customizer controls also stop working.

#23 @johnbillion
12 years ago

Nope, even more specifically, it happens after you've clicked another link after you've clicked a link which results in a server side redirect.

#24 @nacin
12 years ago

We should be able to block/override Location headers as well as hook into wp_redirect() and short-circuit. (Might not prevent the "exit" after one though.)

#25 @nacin
12 years ago

Below is a test case (drop it into mu-plugins) so it can be consistently reproduced.

It sounds like our best bet is to watch for and intercept a 30x in JS. Then check where the redirect is going. If it is going somewhere internally (based on our origins), POST to that URL. If it is going somewhere externally, send us back to the home page with a POST.

It looks like our current check that prevents external links from working also block URLs that are relative, a hash, or a query string. All three of these are allowed in esc_url() and should be considered internal URLs.

add_action( 'get_header', function() {
	echo '<a href="' . esc_url( add_query_arg( 'redirect', 'internal' ) ) . '">Redirect internally</a> &bull; <a href="' . esc_url( add_query_arg( 'redirect', 'external' ) )  . '">Redirect externally</a><br />';
	echo '<a href="/beta/">Root-relative link to home page that gets blocked :-(</a><br />';
	echo '<a href="#footer">Hash link that gets blocked :-(</a><br />';
	echo '<a href="?redirect=internal">Query string link that gets blocked :-(</a>';
} );

add_action( 'template_redirect', function() {
	if ( ! isset( $_GET['redirect'] ) )
		return;

	if ( 'internal' == $_GET['redirect'] )
		wp_redirect( get_permalink( 1 ) );
	else
		wp_redirect( 'http://wordpress.org/' );

	exit;
} );
Version 0, edited 12 years ago by nacin (next)

#26 @nacin
12 years ago

Per http://stackoverflow.com/a/2573589, it is not possible to detect a redirect.

What koopersmith is now working on, is forcing wp_redirect() to return a 200 along with a Location header, then looking for a Location header, testing the target URL, and then either doing a POST (internal or killing the link (external).

There is still going to be an issue with manual header('Location:')'s. We can also hook into shutdown and force a 200 there, but it might be after headers get sent. I suggested we insert some hidden markup into the iframe that can be looked for to ensure the page we got back was generated by the customizer. And if not, we know something went wrong and we can send them to the home page.

Other things we may want to consider: Disabling canonical, cron, and update checks (especially since active theme reporting will be inaccurate).

#27 @koopersmith
12 years ago

In [20861]:

Theme Customizer: Properly handle redirects in the preview by setting wp_redirect_status to 200. props nacin, see #20507, #19910.

#28 @koopersmith
12 years ago

In [20863]:

Theme Customizer: Add a base element to the preview's head element to allow relative links (root, hash, and query strings). see #20507, #19910.

#29 @jane
12 years ago

The Return to Manage Themes link has been removed, in lieu of Save & Activate (or Save & Publish) button returning to the Manage Themes screen after submit, and Cancel button returning to the Manage Themes screen.

#30 @koopersmith
12 years ago

In [20882]:

Theme Customizer: Improve accuracy of identifying internal urls. see #20507, #19910.

The 'customize_preview_link' filter has been replaced by 'customize_allowed_urls'.
Improved accuracy when checking for wp-admin.
Improved accuracy when attempting to match the schemes of the control and preview frames.
Improved accuracy of internal link whitelist.

#31 @nacin
12 years ago

Fixed? Anything left?

#32 @nacin
12 years ago

In [20924]:

Do not spawn cron or trigger update checks when running the customizer.

Both can slow down the experience.

The alternate cron will issue a redirect which creates more work for the
customizer, but not exit immediately, which means shutdown will be delayed
(see future changeset from koopersmith in #20507 where we check for a token
printed on shutdown to ensure the response came from the customizer).

The update checks could also cause bad data to be sent. In particular, the
currently active theme would be incorrect.

see #20507.

#33 @nacin
12 years ago

see future changeset from koopersmith in #20507 where we check for a token

printed on shutdown to ensure the response came from the customizer

That is the only thing left in this ticket.

#34 @koopersmith
12 years ago

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

In [20925]:

Theme Customizer: Add a signature to preview requests to be super-double-ultra-sure that the customizer generated the preview. Redirects can be sneaky. fixes #20507, see #19910.

#4 follow-up: @nacin
12 years ago

In [20926]:

Theme Customizer: Introduce a remove_preview_signature() method that we can employ to ensure we do not think a wp_die() is a customizer-generated preview. see #20507.

#5 @nacin
12 years ago

There have been some inquiries into why we ended up handling cross-domain issues instead of serving the preview via the admin instead of the frontend, or serving the controls via the frontend instead of the admin. Indeed, one strategy was initially decided upon above — serving the controls through the frontend. That wouldn't have worked since we needed not only the admin styles, but also register_setting(), wp-admin auth cookies, and more. But the reverse — serving the preview through the admin — was a very interesting thought.

This wp-admin endpoint to serve the preview would have needed to act like it wasn't a part of the admin for everything but the URL. In theory, it sounded pretty cool, but in practice, it would have had major side effects and would have led us down a rabbit hole of constant compatibility problems. It would have needed to use WP::main() in unusual ways and leveraged the front-end template-loader as well. We would have had to trick WP_Query into thinking it was not querying in the admin. Any theme doing is_admin() {} else {}, or using admin_init, or anything else, could have potentially broke. Any plugins hooking into WP, WP_Query, the template loader, or the template could have potentially been confused.

Mind you, all of this was to avoid something so simple as this: If you are using a mapped domain, un-map it for the customizer.

And so, we agreed to handle cross-domain origin issues the best we could (and given that it works on WP.com, we did exactly that), in order to be as compatible and as flexible as possible. But emphasis would then be placed on the simple solution of simply unmapping the front-end domain when using the customizer.

Solving our cross-domain origin issues allowed us to understand these problems and open up new doors (for example, #21024), and that (plus recommending the unmapping of the domain — hey look, problem solved!) was ultimately way less of a headache than trying to wedge the preview into the admin or vice versa.

(References: An IRC conversation from May 7.)

Note: See TracTickets for help on using tickets.