WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 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 2 years ago.
20337.patch (910 bytes) - added by SergeyBiryukov 2 years ago.
20337.2.patch (2.0 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 benkulbertis2 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.

SergeyBiryukov2 years ago

comment:2 SergeyBiryukov2 years ago

  • Keywords has-patch added; needs-patch removed

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

comment:3 follow-up: koopersmith2 years ago

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

comment:4 in reply to: ↑ 3 azaozz2 years ago

Replying to koopersmith:

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

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

SergeyBiryukov2 years ago

comment:5 follow-up: SergeyBiryukov2 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.

comment:6 in reply to: ↑ 5 ; follow-up: nacin2 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().

comment:7 in reply to: ↑ 6 SergeyBiryukov2 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?

comment:8 koopersmith2 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.