search breaks the search overlay close button
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	• bmansurov
	Jun 18 2015, 11:49 AM

Description

Steps to reproduce:

Visit http://127.0.0.1:8080/wiki/1#/search (Notice the search overlay doesn't open automatically, which is another bug)
Click on the search field that's in the header. The search overlay opens.
Click on the close button of the overlay.
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)

Details

Subject	Repo	Branch	Lines +/-
Use OverlayManager for SearchOverlay	mediawiki/skins/MinervaNeue	master	+62 -39
Correctly close SearchOverlay when the close button is clicked	mediawiki/extensions/MobileFrontend	master	+14 -0
Register #/search with OverlayManager	mediawiki/extensions/MobileFrontend	master	+63 -105

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	Jdlrobson	T101719 [EPIC] Push wikidata descriptions to stable.
Resolved	• bmansurov	T94713 Beta: A user is able to see wikidata descriptions in mobile web search results (in order to learn better select from results)
Resolved	• bmansurov	T101295 Promote current alpha header to beta (for 1 month)
Resolved	• bmansurov	T102946 [Regression] Navigating directly to /wiki/Foo#/search breaks the search overlay close button

Event Timeline

• bmansurov created this task.Jun 18 2015, 11:49 AM

• bmansurov raised the priority of this task from to Needs Triage.

• bmansurov updated the task description. (Show Details)

• bmansurov added a project: Web-Team-Backlog.

• bmansurov subscribed.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 18 2015, 11:49 AM

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:

overlay.mov7 MBDownload

• bmansurov updated the task description. (Show Details)Jun 18 2015, 12:11 PM

• bmansurov set Security to None.

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.

• bmansurov updated the task description. (Show Details)Jun 18 2015, 12:30 PM

phuedx moved this task from Incoming to Old Unorganized Resolved Tasks on the Web-Team-Backlog board.Jun 18 2015, 1:19 PM

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

phuedx added a project: Mobile-Web-Sprint-49-Wayne's-World.

phuedx moved this task from Needs Analysis to To Do on the Mobile-Web-Sprint-49-Wayne's-World board.Jun 18 2015, 1:26 PM

• bmansurov claimed this task.Jun 18 2015, 5:16 PM

• bmansurov moved this task from To Do to Doing on the Mobile-Web-Sprint-49-Wayne's-World board.

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

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

gerritbot added a project: Patch-For-Review.Jun 18 2015, 6:58 PM

• bmansurov moved this task from Doing to Code Review on the Mobile-Web-Sprint-49-Wayne's-World board.Jun 18 2015, 6:59 PM

• bmansurov added a parent task: T94713: Beta: A user is able to see wikidata descriptions in mobile web search results (in order to learn better select from results).

• bmansurov added a parent task: T101295: Promote current alpha header to beta (for 1 month).

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

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

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

Reason:
Apparently, we cannot use this approach for the reason jdlrobson gives. See https://gerrit.wikimedia.org/r/#/c/219249/ for a simpler fix.

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

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.

Jdlrobson moved this task from Code Review to -1 (Needs More Work) on the Mobile-Web-Sprint-49-Wayne's-World board.Jun 18 2015, 9:23 PM

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

• bmansurov moved this task from -1 (Needs More Work) to Code Review on the Mobile-Web-Sprint-49-Wayne's-World board.Jun 19 2015, 10:22 AM

@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 (~JonKatz@tan3.corp.wikimedia.org) 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 (~JonKatz@tan3.corp.wikimedia.org)
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.

Jdlrobson moved this task from Code Review to -1 (Needs More Work) on the Mobile-Web-Sprint-49-Wayne's-World board.Jun 19 2015, 5:55 PM

• bmansurov moved this task from -1 (Needs More Work) to Code Review on the Mobile-Web-Sprint-49-Wayne's-World board.Jun 19 2015, 6:06 PM

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

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

Diffusion mentioned this in rMEXTaedac6fed935: Updated mediawiki/extensions Project: mediawiki/extensions/MobileFrontend….Jun 22 2015, 11:07 AM

• bmansurov mentioned this in rEMFRb16e4c4aa6d1: Correctly close SearchOverlay when the close button is clicked.Jun 22 2015, 11:07 AM

• Jhernandez moved this task from Code Review to Ready for Signoff on the Mobile-Web-Sprint-49-Wayne's-World board.Jun 22 2015, 11:07 AM

• KLans_WMF added a project: Reading-Web-Sprint-50-The-X-Files.Jun 22 2015, 4:25 PM

• Jhernandez moved this task from Needs Analysis to Code Review on the Reading-Web-Sprint-50-The-X-Files board.Jun 22 2015, 4:26 PM

• Jhernandez moved this task from Code Review to -1 (Needs More Work) on the Reading-Web-Sprint-50-The-X-Files board.

• Jhernandez moved this task from -1 (Needs More Work) to Ready for Signoff on the Reading-Web-Sprint-50-The-X-Files board.

https://gerrit.wikimedia.org/r/219249 (branch master): WMF-deploy-2015-06-23_(1.26wmf11)

Jdlrobson removed a project: Reading-Web-Sprint-50-The-X-Files.Jun 23 2015, 12:32 AM

Verified on BC.

phuedx moved this task from Ready for Signoff to Done on the Mobile-Web-Sprint-49-Wayne's-World board.Jun 25 2015, 1:27 PM

phuedx removed a project: Patch-For-Review.Jun 25 2015, 1:41 PM

Legoktm removed a subscriber: • Forrestbot.Jun 29 2015, 5:49 PM

• Phabricator_maintenance moved this task from Old Unorganized Resolved Tasks to 2014-15 Q4 on the Web-Team-Backlog board.Mar 7 2018, 1:24 AM

Jdrewniak mentioned this in T189212: [Bug] Search overlay doesn't always respect back button.Mar 27 2018, 10:32 AM

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

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

gerritbot added a project: Patch-For-Review.Mar 27 2018, 10:51 AM

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