Page MenuHomePhabricator

[Regression] Navigating directly to /wiki/Foo#/search breaks the search overlay close button
Closed, ResolvedPublic


Steps to reproduce:

  1. Visit (Notice the search overlay doesn't open automatically, which is another bug)
  2. Click on the search field that's in the header. The search overlay opens.
  3. Click on the close button of the overlay.
  4. Depending on where you pasted the link in step #1, you'll get two results, both of which are wrong.
    • If you visited the url in step #1 in a new tab, you'll see a blank tab.
    • If you visited the url in step #1 in a tab where you were viewing some page, you'll be taken to that page.

Expected behavior for #4:
The darn overlay should just disappear.

This bug happens on (possibly other browsers/OSes too):

  • Google Chrome Version 43.0.2357.124 (64-bit)
  • Mac OS X 10.10.3 (14D136)

Event Timeline

bmansurov raised the priority of this task from to Needs Triage.
bmansurov updated the task description. (Show Details)
bmansurov added a project: Readers-Web-Backlog.
bmansurov added a subscriber: bmansurov.

I can't reproduce this in stable, beta, or alpha using the latest MobileFrontend master (e2c451b). Regarding #4: if I paste the URL into a new tab, then I am taken to the page that I expect.

I'm using Google Chrome (43.0.2357.124).

However, I do see #/search not opening the search overlay automatically.

Here is a screencast:

I cleared the browser cache a few times (apparently) and have been able to reproduce the bug.

Thanks for the screencast @bmansurov. I understood the report, I just couldn't reproduce it. In future could you provide the details that you highlighted at the end of the video (browser and OS versions) in the bug report itself? Ta.

phuedx renamed this task from Can't close the search overlay to [Regression] Navigating directly to /wiki/Foo#/search breaks the search overlay close button.Jun 18 2015, 1:22 PM

Change 219225 had a related patch set uploaded (by Bmansurov):
Register #/search with OverlayManager

Change 219249 had a related patch set uploaded (by Bmansurov):
Correctly close SearchOverlay when the close button is clicked

Change 219225 abandoned by Bmansurov:
Register #/search with OverlayManager

Apparently, we cannot use this approach for the reason jdlrobson gives. See for a simpler fix.

Notice the search overlay doesn't open automatically, which is another bug

Turns out this is by design. Say I searched for something and clicked on a search result. Then I hit the browser's back button. Voila, I see an empty search overlay. This overlay either should be pre-populated with the previous search results or not shown at all.

Barry is giving false alarms. See my comment in the patch.

@Jdlrobson, can you reply to my comment in the patch?

replied in irc:

10:52 AM <jdlrobson> bmansurov: wanna chat about the overlay stuff?
10:52 AM <bmansurov> yes
10:53 AM <jdlrobson> So if you look at resources/mobile.startup/OverlayManager.js
10:53 AM <jdlrobson> you'll see there is an _om_hide event being emitted on every hide
10:53 AM <jdlrobson> (hide event)
10:53 AM <jdlrobson> and _hideOverlay called hide
10:53 AM <jdlrobson> so the reason the tests are failing is if you call overlay.hide() on something using the OverlayManager it calls hide twice
10:54 AM <jdlrobson> It's messy but this is why the browser tests are failing i suspect :)
10:54 AM <jdlrobson> so we probably want some option like usingOverlayManager: true
10:54 AM <jdlrobson> i'm not sure what the best architecture is there
10:54 AM <bmansurov> jdlrobson: i see. but does removing window.history.back() make sense? i don't see a reason to keep it.
10:54 AM <jdlrobson> i try to avoid overlay hiding/overlay manager as much as possible
10:55 AM <jdlrobson> bmansurov: not really because you don't know where they are coming back from
10:55 AM <jdlrobson> for instance say there was a link in the talk overlay to the editor
10:55 AM <jdlrobson> personally i wouldn't touch the default behaviour - you'll cause yourself more trouble than it's worth.
10:55 AM <bmansurov> ok, make sense. I think i'll do what ImageOverlay does
10:55 AM ⇐ T13|mobile quit (~{{Guy}}@Wikimedia/Technical-13)
10:55 AM <jdlrobson> i must confess i haven't even looked at the bug you are trying to fix
10:56 AM <jdlrobson> it would be a good idea to get a sense of urgency since i don't think it went triaged
10:56 AM <bmansurov> it's been reported before but only fixed with the image overlay
10:56 AM ⇐ JonKatz quit ( Quit: My Mac has gone to sleep. ZZZzzz…
10:56 AM <jdlrobson> it doesn't seem as important to fix as the templating stuff for instance
10:56 AM kaity|away → kaity
10:56 AM <bmansurov> when I picked it up it was at the top of the pile
10:57 AM <jdlrobson> yeh i can imagine.. moving columns adds a card to top.
10:57 AM → JonKatz joined (
10:57 AM <jdlrobson> but Barry is not giving false alarms
10:58 AM <jdlrobson> he passes without your patch but doesn't with it.

Change 219249 merged by jenkins-bot:
Correctly close SearchOverlay when the close button is clicked

Change 420727 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Use OverlayManager for SearchOverlay

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