Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#36613 closed defect (bug) (fixed)

Browser back button doesn't work in Theme Browser

Reported by: dd32's profile dd32 Owned by: adamsilverstein's profile adamsilverstein
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch commit
Focuses: javascript Cc:

Description

When you're in the Add New Theme Browser, if you enter a details view and then press the browser back button, nothing happens.
The URL changes (as we appear to be using pushstate) however the state of the screen remains the same.

Using the Theme Browser for already installed themes, the browser back button works as expected, it appears to be limited to the new themes JS.

Attachments (8)

36613.diff (2.3 KB) - added by adamsilverstein 7 years ago.
36613.2.diff (2.4 KB) - added by adamsilverstein 7 years ago.
36613.3.diff (1.0 KB) - added by adamsilverstein 7 years ago.
36613.4.diff (1.8 KB) - added by adamsilverstein 7 years ago.
36613.5.diff (1.8 KB) - added by adamsilverstein 7 years ago.
36613.6.diff (2.5 KB) - added by adamsilverstein 7 years ago.
36613.7.diff (2.5 KB) - added by adamsilverstein 7 years ago.
36613.8.diff (2.7 KB) - added by adamsilverstein 7 years ago.

Download all attachments as: .zip

Change History (33)

#1 @nobremarcos
7 years ago

OS and browser version? can't seem to replicate the issue.

#2 @dd32
7 years ago

Chrome 55.0.2883.95, Mac.

To be specific; Visit the add new themes, head into Latest, open a theme details/preview, tab back and forth between themes using the < and > buttons, click browser back, preview remains open, url changes, etc.

#3 @adamsilverstein
7 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

36613.diff
Themes: Fix issues when navigating browser history in the theme installer. Show correct theme and close previewer when backing out to install page.

  • Store a reference to the current preview when the theme installer preview view is opened
  • Add a listener to the preview so we can send it close events more easily
  • When navigating themes with next/back and adding the url to the browser history, use replace: false so we build up some history
  • When going back all the way to main install screen (the sort route), close the preview if open so view is back to the theme browser

@dd32 can you please review and see if this works for you?

Here is a screencast testing locally for the record: https://cl.ly/1z2r2G0c0z0q

#4 @dd32
7 years ago

@adamsilverstein In my testing it seemed to work well - I say commit it.

I didn't discover this directly myself, I was watching someone else attempt to install a theme who got confused by the previous behaviour - I'm going to try to do another look-over-the-shoulder test, just to ensure things seem right.

#5 @adamsilverstein
7 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

36613.2.diff is a slight improvement: Open the theme preview when reloading or linking to a theme install route.

Enables (reloading or) linking to a specific theme install preview, for example .../wp-admin/theme-install.php?theme=twentyseventeen. To test, try navigating to a specific theme, then hit refresh, the page should open to the same state after the latest patch.

#6 @adamsilverstein
7 years ago

  • Milestone changed from Awaiting Review to 4.8

#7 @adamsilverstein
7 years ago

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

In 40107:

Themes: enable browser history support in add new theme screen.

Enable history support for the new theme screen, including navigating theme details and closing the details modal. Theme selection is now also bookmark-able, so linking to a URL like /wp-admin/theme-install.php?theme=twentyseventeen correctly opens the theme preview.

Props dd32.
Fixes #36613.

#8 @afercia
7 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Noticed a JS error while navigating through themes, sorry for reopening 😬 To reproduce:

  • Add Themes screen > click on a theme, let's say the first one
  • the theme installer opens
  • navigate a couple themes forward
  • then use your browser's back button to navigate backwards
  • after the third click, the theme installer correctly closes
  • now click on another theme in the same screen, let's say the second one
  • the theme installer opens
  • click on the theme installer next or previous arrows
  • no navigation happens

Here's what I get in the console:

Uncaught TypeError: Cannot read property 'at' of undefined
theme.js?ver=4.8-alpha-39357-src:526

#9 @adamsilverstein
7 years ago

@afercia thanks for catching this, I'll take a look.

#10 @adamsilverstein
7 years ago

@afercia 36613.3.diff fixes this issue, altough it is a bit of a kludge.

The issue at hand is triggering the close of the overlay/preview when using browser history to reach the new theme page (could be back or forward). The new code simulates a click on the close button and fixes the issue where the model underlying the preview gets reset incorrectly. I wanted to put this up for review and also keep trying to determine a better approach.

#11 @adamsilverstein
7 years ago

  • Keywords has-patch added; needs-patch removed

@afercia I found the underlying issues - the preview event handlers weren't being unbound when you hit the back button and the preview was recreated each time. can you please give 36613.4.diff a test?

Thanks.

#12 @afercia
7 years ago

@adamsilverstein thanks, will do! (tomorrow)

#13 follow-up: @afercia
7 years ago

@adamsilverstein finally tested a bit, yep previous error seems gone. However, there are still edge cases where seems it's a bit difficult to make the history work correctly. To reproduce one of these cases:

  • start from the Popular tab (?browse=popular)
  • click on the second theme
  • the installer opens, navigate 2 themes forward
  • click on the browser's Back button three times, and the installer closes
  • you're back on the Popular tab
  • notice your browser's Forward button is disabled, maybe should keep the forward history
  • notice in the browser address bar ?browse=popular is gone

Now start a new theme navigation:

  • click on the third theme
  • the installer opens, navigate 2 themes forward
  • click on the browser's back three times, and the installer closes
  • you're now on the Featured tab, you should be on the Popular one
  • notice your browser's Forward button is enabled and clickable
  • click on the browser's Forward button
  • the address bar gets updated with the last theme displayed parameter ?theme=themename
  • nothing happens but you get a JS error Cannot read property 'toJSON' of undefined
  • refresh the page, and the theme installer opens

I think some of the issues come from underlying issues in the theme installer, but haven't looked in depth.

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

Replying to afercia:

@adamsilverstein finally tested a bit, yep previous error seems gone. However, there are still edge cases where seems it's a bit difficult to make the history work correctly. To reproduce one of these cases:

Ha, you are a pro edge case finder. The current routing doesn't include browse= query var, however it is a good idea to add it if possible. I will take a look and try to improve the behavior here.

#15 @adamsilverstein
7 years ago

  • Keywords needs-refresh added

I am going to work on addressing these final issues since the primary fix adding history here is already in 4.8.

This ticket was mentioned in Slack in #core by flixos90. View the logs.


7 years ago

#17 @adamsilverstein
7 years ago

  • Keywords needs-refresh removed

@afercia can you please retest with 36613.7.diff - this should resolve the issues you were seeing when using the pupular and other tabs.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


7 years ago

#19 @afercia
7 years ago

@adamsilverstein yep tested a bit and couldn't reproduce the issues with tabs, which is a good thing :) Technically it works.
However, I'm wondering about user expectations in one specific case:

  • browse a bit through tabs: popular, featured, new
  • then click on a theme details
  • the installer opens
  • navigate forth and back through themes
  • at some point, close the installer clicking on the "X" button

At that point, as a user, I would expect the browser Back button to navigate through the tabs I previously visited. Instead, and it's technically correct, it opens the installer and walks through history.
That's technically correct because history includes the visited themes preview. By the way, maybe users would expect to be back to a "normal" navigation once they explicitly close the installer.
Not sure it that makes sense, I'd like to hear what other people feel about this.

#20 @adamsilverstein
7 years ago

@afercia thanks for testing. i think this is the expected behavior - i will give it another test.

#21 @adamsilverstein
7 years ago

@afercia - I gave this a little more testing, I think this is the correct behavior - essentially every time you change the theme, and when you close the theme browser, the new state gets pushed onto the browser history - you will see the browser url update with each change you make. after you close the theme browser and you start clicking thru history, each state should be restored in the exact sequence you performed them. So hitting back after closing the theme browser would re-open it.

The only slight issue i saw was around the featured tab which is also the default tab, i'll test this a little more.

Here is a screencast of me testing the steps you described:

https://cl.ly/2s1P3I0f0t1O

#22 @afercia
7 years ago

@adamsilverstein overall I think it's a very nice improvement that should definitely go in :) My previous concern was just about users expectations, but that's largely based on assumptions. If needed, details can be refined later, I think. So definitely +1 from me.

#23 @adamsilverstein
7 years ago

  • Keywords commit added; needs-testing removed

thanks again for all the testing @afercia, i will get this committed shortly.

36613.8.diff is a slight refinement that handles the default tab state better. 'Featured' was always the default tab, the updated code now ajdusts the browser url on initial load to reflect this, without adding to browser history. the lets us remove some special handling for the 'featured' tab and fixes an edge case where an extra step was introduced into history.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


7 years ago

#25 @adamsilverstein
7 years ago

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

In 40824:

Themes: improve browser history support on new themes page.

When closing the theme preview, restore the previously selected tab. Avoid an issue where duplicate entries in the history prevented navigation. When re-opening the preview, remove bound event handlers before re-adding them.

Props afercia.
Fixes #36613.

Note: See TracTickets for help on using tickets.