Page MenuHomePhabricator

Can't close main menu on special pages
Closed, ResolvedPublic

Description

Doh. @phuedx.
Seems fine on non-special pages...

Event Timeline

Jdlrobson created this task.May 7 2015, 9:48 PM
Jdlrobson raised the priority of this task from to High.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: Readers-Web-Backlog.
Jdlrobson moved this task to Incoming on the Readers-Web-Backlog board.
Jdlrobson added subscribers: Jdlrobson, phuedx.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 7 2015, 9:48 PM
phuedx added a comment.May 8 2015, 9:35 AM

@Jdlrobson: Same browser/OS as last time?

Nope. All browsers now. Was seeing this in chrome.

kaldari claimed this task.May 8 2015, 6:29 PM

Special:EditWatchlist, Special:Gather, and Special:History seem to work fine, but Special:MobileOptions seems to be broken.

Special:Nearby too
Maybe related to loading in head?

It looks like there are two bugs:

  1. Clicking on the non-menu part of the page doesn't close the menu (which has been broken since at least March)
  2. Clicking on the hamburger icon doesn't close the menu (which just broke recently)

It looks like bug #1 was caused by https://gerrit.wikimedia.org/r/#/c/188483/ back in February. The change assumes that the 'mobile.startup' module is loaded after '#mw-mf-viewport'
has been added to the page, which isn't always the case. Should be OK to revert this change from the looks of it.

2 is due to https://gerrit.wikimedia.org/r/#/c/207005/11/resources/mobile.mainMenu/MainMenu.js which I thought I flagged to @bmansurov during review. "lose the main menu on page center click regardless of whether the main menu button is clicked or not."

(the result of this is clicking the main menu triggers 2 toggle events due to event bubbling resulting in the menu staying open)

  1. I think the reason is that there is no ".transparent-shield cloaked-element" on Special:MobileMenu. I wonder why not?
  2. Hamburger is part of the page center which and is treated as every other part of the page center. Fixing 1 should fix this too. And it's not caused by the patch @Jdlrobson mentions.

Actually reverting https://gerrit.wikimedia.org/r/#/c/188483/ causes all sorts of issues since modules have started assuming that the skin is available immediately (rather than on DOM ready). This may be tricky to figure out without unraveling some things.

@bmansurov The reason there is no ".transparent-shield cloaked-element" on Special:MobileMenu is because the DOM hasn't actually been rendered on that page at the point that postRender in Skin.js runs.

@kaldari, that's what I figured. But the patch you linked needs to be done that way because 'skin' module won't be available sooner otherwise. The solution is to wait until the DOM is ready and insert the transparent shield. I'm working on it.

The reason the DOM hasn't loaded is because both mobile.special.nearby.scripts and mobile.special.mobileoptions.scripts are set to position:top so they load in the head. This, in turn, is to prevent a flash of unstyled content.

@Jdlrobson @bmansurov It seems weird to me that we have postRender() running before the DOM is actually rendered. Shouldn't we fix that instead?

Yes, @kaldari, but that alone doesn't fix the problem for some reason.

@bmansurov Are you sure? When I put the initialization of 'skin' in an init function that runs on DOM ready, it fixes the problem for me (but causes other problems). Also changing mobile.special.nearby.scripts and mobile.special.mobileoptions.scripts to position:bottom also fixes the problem for me (but causes flash of unstyled content).

@kaldari, I meant putting the postRender contents inside DOM ready. Looking at the View code I can see that it assumes that this.el is present on page. I'm not sure if that was the intended behavior.

Change 209849 had a related patch set uploaded (by Bmansurov):
Make Skin work correctly when run from the head of the document

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

bmansurov claimed this task.May 9 2015, 1:35 AM
bmansurov added a subscriber: kaldari.

Change 209849 merged by jenkins-bot:
Make Skin work correctly when run from the head of the document

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

@kaldari: I've moved this to Ready for Signoff to remind us to SWAT deploy this.

phuedx closed this task as Resolved.May 19 2015, 1:13 PM