Page MenuHomePhabricator

[Bug] Search overlay doesn't always respect back button
Closed, ResolvedPublic3 Story Points

Description

Steps to reproduce

  1. Go to https://en.m.wikipedia.org/wiki/Barack_Obama
  2. Tap search and press "a" (now at Barack_Obama#/search)
  3. Press the browser back button (now at Barack_Obama)
  4. Press the browser forward button (now at Barack_Obama#/search)
  5. Tap search and press "a" (now at Barack_Obama#/search)
  6. Press the browser back button (now at Barack_Obama)

OR:

  1. Go to https://en.m.wikipedia.org/wiki/Barack_Obama
  2. Tap search and press "a" (now at Barack_Obama#/search)
  3. Remove a watchlist star (or add and remove a star).
  4. Press the browser back button (now at Barack_Obama)

Expected results

  • Search overlay is dismissed when URL fragment isn't #/search

Actual results

  • Search overlay persists until closed via the cross button or browser refresh

Acceptance criteria

  • If the address bar has #/search and I refresh the page, SearchOverlay will remain present
  • If SearchOverlay is open and I click browser back button, hide the SearchOverlay and ensure #/search is removed from URL
  • If SearchOverlay is open and I click X (close) button, hide the SearchOverlay and ensure #/search is removed from URL
  • If I am on page A, I do a search, click a search result B and click back I should be taken to the article A without the search open
  • If I am on page D, click back and land on page C, open the SearchOverlay, close the search overlay, click Forward, I should be taken to page D (not the SearchOverlay)
  • If I click the search input, click browser back and click browser forward the SearchOverlay should show (unless there is an article forwards in the browser history, which should take priority)

Environments observed

Browser Version:

  • Chromium v64.0.3282.167 (Official Build) Built on Ubuntu , running on Ubuntu 17.10 (64-bit)
  • Firefox v58.0.2 (64-bit)

OS Version:

  • Ubuntu v17.10

Device Model:

  • Desktop

Device Language:

  • English

also affecting mobile web: iOS (Safari & Chrome), Android (Chrome)

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 8 2018, 3:22 PM
Niedzielski updated the task description. (Show Details)Mar 8 2018, 3:28 PM
Jdlrobson added subscribers: alexhollender, Nirzar, Jdlrobson.

This has been the case since the beginning of time: I1c657fd46a2b5be4f27aa508a5cc0d946d6b98a8

From what I remember:

  • The SearchOverlay should not be persistent. Eg. navigating to #/search should not load it.
  • However.. it should close when the user clicks a back button.
  • However it should not show again if the user clicks forward or clicks a URL which contains #/search.
  • If I do a search and then click back I should be taken to the article NOT the search interface.

Point 2 is why we update the hash fragment and it becomes inconsistent elsewhere.

@Nirzar / @alexhollender could you help us review this behaviour and define how this should behave?

confirmed that this is also an issue on mobile web

@Jdlrobson your comments above look good to me. I think that's a good description of the desired behavior.

Should I assign this to one of you?

alexhollender removed alexhollender as the assignee of this task.Mar 13 2018, 9:07 PM
alexhollender moved this task from Design to Triaged but Future on the Readers-Web-Backlog board.
ovasileva triaged this task as Normal priority.Mar 14 2018, 10:10 AM
ovasileva moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.
ovasileva set the point value for this task to 3.Mar 14 2018, 4:10 PM
Jdlrobson updated the task description. (Show Details)Mar 14 2018, 5:33 PM

Just FYI to whoever works on this 3 seems a little optimistic :)
We're basically wrestling with the address bar here and the overlay manager was not setup to work this way.

I suspect we'll need to explore using replaceState (and update mediawiki.router to support that)

I agree with @Jdlrobson that to get this working, replaceState seems like the appropriate solution.
While looking at this problem though, I noticed that visiting a page with the #/search url, like https://en.m.wikipedia.org/wiki/Barack_Obama#/search doesn't actually trigger the search overlay either. Seems like that should be fixed too?

I agree with @Jdlrobson that to get this working, replaceState seems like the appropriate solution.
While looking at this problem though, I noticed that visiting a page with the #/search url, like https://en.m.wikipedia.org/wiki/Barack_Obama#/search doesn't actually trigger the search overlay either. Seems like that should be fixed too?

Agreed - we can add that to the task and try to re-estimate today.

Jdlrobson added a comment.EditedMar 20 2018, 2:19 PM

Per acceptance criteria search should not show in that circumstance.... ping @Alex @Nirzar

Change 420727 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] [WIP] Triggering search overlay through mediawiki.router

https://gerrit.wikimedia.org/r/420727

@Jdlrobson @Niedzielski — chatted with @Jdrewniak and was thinking of updating the acceptance criteria to the following (changes are in bold):

  • If the address bar has #/search and I refresh the page, SearchOverlay will remain present
  • If SearchOverlay is open and I click browser back button, hide the SearchOverlay and ensure #/search is removed from URL
  • If SearchOverlay is open and I click X (close) button, hide the SearchOverlay and ensure #/search is removed from URL
  • If I am on page A, I do a search, click a search result B and click back I should be taken to the article A without the search open
  • If I am on page D, click back and land on page C, open the SearchOverlay, close the search overlay, click Forward, I should be taken to page D (not the SearchOverlay)
  • If I click the search input, click browser back and click browser forward the SearchOverlay should show (unless there is an article forwards in the browser history, which should take priority)

Thoughts?

@alexhollender the behaviour that SearchOverlay shows when #/search is present sounds fine. One word of caution - if a user shares a page with #/search in the URL there may be no way to close the overlay... but this is the same for the editor (https://en.m.wikipedia.org/wiki/Barack_Obama#/editor/0) - this is currently a limitation we have to work with.

@Jdrewniak mediawiki.router currently has no support for replaceState but we can add it provided we write tests... (T189173)

@Jdlrobson thanks for reviewing. My hope is that if we're better about ensuring that #/search gets removed when the SearchOverlay is closed, it will be less common for people to share with #/search in the URL.

@Jdrewniak I went ahead and updated the acceptance criteria in the ticket.

If the address bar has #/search and I refresh the page, SearchOverlay will remain present

I'm trying out @Jdrewniak's changes and this seems like good functionality but it's unbecoming when the results are not preserved. It almost look like an error. Consider the following case:

  1. User visits https://en.m.wikipedia.org/wiki/Barack_Obama.
  2. User taps search.
  3. User types "university" and some results are shown.
  4. User presses back and sees that "University of Hawaii" is what they're looking for.
  5. User presses forward to complete their search.

As a user, I would expect my previous query (and hopefully results) to be preserved so that all I'd have to type is "of hawaii". Instead, I see this rather confusingly empty screen and must retype the whole query:

I'm not sure what's in scope for this ticket as a bug fix vs feature, but I wanted to clarify that if we persist search in navigation state, we may also want to 1) persist at least the query 2) improve the empty input screen with a graphic of some kind.

Hi guys, it's hard to explain back button behaviour using words, so I made a short video explaining (to myself) what I thought the ideal back button behaviour was.
https://drive.google.com/file/d/1hb5nqxGCwUwgeMPidvsAqdwR36csWHme/view?usp=sharing

Basically my first instinct was to use history.back() to close the overlay, but I found out this has problems T102946.

I still think having the router trigger the overlay, and not the overlay trigger the router (as is currently) is the more correct solution (it also allows us to use the overlayManager), but having the overlay close the page when it's open on pageload is not acceptable either. So, idea: don't show the search overlay on pageload, even if the #/search hash is present.
Like this:
https://drive.google.com/file/d/1LrhEoAi2fvwgD2R_n-ITz0ECvkOZmG5U/view?usp=sharing

This behaviour is actually consistent with @alexhollender's initial idea for how the search overlay should work - the rationale being that in case someone shares a url with the search overlay open, the user opening that link would see the article, and not the overlay. This approach would prevent that by intentionally removing the search overlay on pageload.

@Niedzielski re:clearing search results - I agree, I added an clearResults call to my initial patch, but it turns out that if you just let the browser do its thing, it actually does maintain the state of the search results when you click the back & forward buttons.

Change 422118 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Registering SearchOverlay with OverlayManager

https://gerrit.wikimedia.org/r/422118

pmiazga claimed this task.Apr 3 2018, 6:10 PM

@alexhollender code is up on staging for you to test prior to any merging. Let us know what you think.

@Jdlrobson in terms of testing: is there any way to get search results to show?

As far as I can tell the original issue has been fixed 🙌

There was this criteria which I added at some point after we started working on this, which is perhaps a bit of a stretch-goal:

  • If I am on page D, click back and land on page C, open the SearchOverlay, close the search overlay, click Forward, I should be taken to page D (not the SearchOverlay)

I don't think this is happening. The only downside I see here is that if I to to page A, then B, then C, then D, then use the back button to go < back C < back B < back A, then open the search overlay, I loose the ability to then use the forward button to go forward > B forward > C forward > D. But honestly I might just be down way too far in the weeds here, finding weird nuanced problems that aren't really problems...

alexhollender updated the task description. (Show Details)Apr 4 2018, 1:29 PM
pmiazga removed pmiazga as the assignee of this task.Apr 4 2018, 5:01 PM
pmiazga added a subscriber: pmiazga.

@alexhollender I've fixed staging. Search results should show now!

Code has not been +2ed because I need a design review. Alex let me know when that's happened so we can get the code merged. I'll pass to Anthony for QA when that's done!

Change 420727 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Use OverlayManager for SearchOverlay

https://gerrit.wikimedia.org/r/420727

Change 422118 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Registering SearchOverlay with OverlayManager

https://gerrit.wikimedia.org/r/422118

Since it wasn't specified - please test this on the beta cluster.

Looks good on beta. Tried multiple devices and browsers (ios, android - chrome, firefox, safari) and did not see the overlay problem.

ovasileva closed this task as Resolved.Apr 10 2018, 10:44 AM

looks good, thanks all!