Page MenuHomePhabricator

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


Steps to reproduce

  1. Go to
  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 ( 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 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.

Niedzielski updated the task description. (Show Details)Mar 16 2018, 1:21 PM
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

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

I believe this got fixed by the above patch.

I think so too!

Niedzielski added a subscriber: ABorbaWMF.

Over to you, @ABorbaWMF. Please test this on the beta cluster:

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