Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#20337 closed defect (bug) (fixed)

On theme customizer, back button should close preview mode

Reported by: jane Owned by: koopersmith
Milestone: 3.4 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

From #19910:

Should browser back button close Theme Customizer rather than return to a previous page?

It should return to Install/Manage Themes.

Attachments (3)

customize-loader.dev.js.patch (892 bytes) - added by benkulbertis 10 years ago.
20337.patch (910 bytes) - added by SergeyBiryukov 10 years ago.
20337.2.patch (2.0 KB) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (11)

#1 @benkulbertis
10 years ago

I made an attempt at this using hashes, however I don't quite have it. It works as expected the first run through, but after clicking the back button and having the overlay close something is not reset correctly because the "Customize" link no longer functions. Maybe someone can build from the code I wrote thus far.

#2 @SergeyBiryukov
10 years ago

  • Keywords has-patch added; needs-patch removed

20337.patch builds on Ben's patch. Seems to work fine in my testing.

#3 follow-up: @koopersmith
10 years ago

I haven't had a chance to test these patches yet, but I'd rather we use the window.history API.

#4 in reply to: ↑ 3 @azaozz
10 years ago

Replying to koopersmith:

+1 for using window.history if possible or pop push state.

Last edited 10 years ago by azaozz (previous) (diff)

#5 follow-up: @SergeyBiryukov
10 years ago

Not sure how to use windows.history in this context, since it doesn't allow changes to history.

20337.2.patch is another attempt. After clicking browser's Back button, Forward button works as well (returns to Theme Customizer).

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

The patch uses onhashchange rather than onpopstate, since the latter is not supported by IE 8.

The if ( Loader.iframe ) check in line 68 is due to Loader.close() being called twice when clicking .close-full-overlay:

  1. Directly in this.element.click.
  2. Via window.onhashchange.

It would probably be a bit cleaner to remove the call from this.element.click and let onhashchange handle the closing.

#6 in reply to: ↑ 5 ; follow-up: @nacin
10 years ago

Replying to SergeyBiryukov:

Not sure how to use windows.history in this context, since it doesn't allow changes to history.

I think window.history was a reference to history.pushState().

#7 in reply to: ↑ 6 @SergeyBiryukov
10 years ago

Replying to nacin:

I think window.history was a reference to history.pushState().

Ah, thanks. I thought of pushState(), but was confused by the fact that it's not used anywhere in core yet. Seems replaceState() was once added in [16615], but then removed in [18028], along with list-table.dev.js.

Wouldn't using pushState() introduce an inconsistent behaviour between browsers, since it's not supported by IE 9? Or should we bring in some external library or another workaround as a fallback?

#8 @koopersmith
10 years ago

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

In [20488]:

Theme Customizer: Integrate with browser history. Use window.history by default, with window.onhashchange as a fallback. fixes #20337, see #19910.

Note: See TracTickets for help on using tickets.