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
    Screenshot from 2018-03-12 16-55-51.png (918×1 px, 214 KB)
  2. Tap the search button
    Screenshot from 2018-03-12 16-55-57.png (918×1 px, 26 KB)
  3. Tap the close button
    Screenshot from 2018-03-12 16-56-02.png (918×1 px, 214 KB)
  4. Tap the search button
    Screenshot from 2018-03-12 16-56-08.png (918×1 px, 212 KB)
    (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
    Screenshot from 2018-03-12 16-56-17.png (918×1 px, 212 KB)

Expected results

  • Search overlay is fullscreen

Actual results

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

Screenshot from 2018-03-12 16-56-19.png (918×1 px, 212 KB)

Screenshot from 2018-03-12 16-56-35.png (918×1 px, 178 KB)

Screenshot from 2018-03-16 08-19-33.png (756×932 px, 227 KB)

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

ovasileva triaged this task as Medium 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.

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

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

looks like we're all done here