#20507 closed defect (bug) (fixed)
3.4 Preview/Customize page "Return to Manage Themes" link doesn't work as expected
Reported by: | TomAuger | Owned by: | 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)
Change History (44)
#4
follow-up:
↓ 5
@
13 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
@
13 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
@
13 years ago
- Severity changed from major to blocker
Per koopersmith, this should be considered a blocker for beta 4.
#7
follow-up:
↓ 8
@
13 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
@
13 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.
#10
@
13 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
@
13 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
@
13 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.
#13
@
13 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.
#14
@
13 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.
#16
in reply to:
↑ 15
@
13 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.
#20
@
13 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
This is still broken. Steps to reproduce:
- Visit the theme customizer and change some settings if you feel like it.
- Click a link to another page on the site.
- 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.
#22
@
13 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
@
13 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
@
13 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
@
13 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> • <a href="' . esc_url( add_query_arg( 'redirect', 'external' ) ) . '">Redirect externally</a><br />'; echo '<a href="/">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; } );
#26
@
13 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).
#29
@
13 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.
#33
@
13 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.
#5
@
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.)
Tested in Firefox 11, Chrome 18, IE 8, Opera 11.62, Safari 5.1.5. Reproduced in Chrome and Safari.