Opened 9 years ago
Closed 8 years ago
#36613 closed defect (bug) (fixed)
Browser back button doesn't work in Theme Browser
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (33)
#2
@
8 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
@
8 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
@
8 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
@
8 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.
#8
@
8 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
#10
@
8 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
@
8 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.
#13
follow-up:
↓ 14
@
8 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
@
8 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
@
8 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.
8 years ago
#17
@
8 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.
8 years ago
#19
@
8 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
@
8 years ago
@afercia thanks for testing. i think this is the expected behavior - i will give it another test.
#21
@
8 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:
#22
@
8 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
@
8 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.
OS and browser version? can't seem to replicate the issue.