Page MenuHomePhabricator

[Bug] Search overlay isn't always full screen
Closed, ResolvedPublic

Description

Steps to reproduce

  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page
  2. Tap the search button
  3. Tap the close button
  4. Tap the search button
    (you'll probably see the screen kind of jank out here like a vertical scroll bar flipped on for a moment)
  5. Tap the close button

Expected results

  • Search overlay is fullscreen

Actual results

  • Search overlay is sadly not fullscreen 😭 It usually goes full screen when search results are shown:


Environments observed

Browser Version:

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

OS Version:

  • Ubuntu v17.10

Device Model:

  • Desktop

Device Language:

  • English (beta cluster)

Developer notes

  • All overlays are full screen by default - there's even a Overlay.prototype.fullScreen flag. An "overlay-enabled" class is added to the html tag when an overlay is shown and removed when it is hidden. Overlay::hide and Overlay::show manage this.

In the case of the SearchOverlay when the search button is clicked a second time a show AND hide action are called.
The hide action is incorrectly called due to SearchOverlay::_hideOnRoute. The method knows it's doing something naughty - because it has a FIXME. (FIXME: Remove when search registers route with overlay manager)

It looks like @Jdrewniak is working to remove this (https://gerrit.wikimedia.org/r/#/c/422118/5/resources/mobile.search/SearchOverlay.js) in which case his changes in T189212 will fix this.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 12 2018, 10:09 PM
ovasileva triaged this task as Normal priority.Mar 13 2018, 11:02 AM
Jdlrobson added subscribers: Nirzar, Jdlrobson.

I'm pretty sure this is by design ,right @Nirzar ?

@Jdlrobson nah this is just some inconsistent behaviour of going inside search for the first time vs the second time. seems like a bug

Let's do a bit of analysis on this and work out what's happening here.

Jdlrobson updated the task description. (Show Details)Apr 3 2018, 9:53 PM
Jdlrobson added a subscriber: Jdrewniak.

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

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

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

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

I believe this got fixed by the above patch.

Niedzielski added a subscriber: ABorbaWMF.

Over to you, @ABorbaWMF. Please test this on the beta cluster: https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page.

Looks good to me on the major browsers. Popping over to design for review.

alexhollender added a subscriber: alexhollender.
ovasileva closed this task as Resolved.Apr 30 2018, 5:17 PM

looks like we're all done here