Page MenuHomePhabricator

Append mw.notify div to the body instead of content so it is always on top
Closed, ResolvedPublic

Description

mw.notify notifications are considered sort of "global" notifications for tools and extensions - and they should appear on top of everything else.

Many tools and extensions (MobileFrontend is a noticeable example) use components (like overlays) outside the content div, which means that they appear on top of the mw.notify div and hide it. Moving the notification div to the top seems like a good idea.

Quoting @Krinkle 's concerns (and discussion) from the task:

I'm for moving it to the body. However two things to consider:

  • The positioning of the notification container is intentionally relative to the content area so that it doesn't overlap with personal tools and search (Vector/MonoBook skin). It only moves to an absolute position if the user is scrolled down. Be sure to verify this still works as intended.
  • I'm not convinced that it is intentional to have notifications go over top of modal dialogs. I recall several gadgets that bring things to the attention of the user via non-autohide notification bubbles which then have a clickable button in it that opens a modal. I suppose we can still make this change with the expectation that such an application will programmatically hide the notification once the button is clicked - for the benefit of other (autohide) notifications being visible (instead of being trapped behind the modal and expiring before the user sees them).

This second point is probably worth a release note and notice to developers. Maybe create a task and tag it with Notice and Developer-notice, and make sure it gets into Tech News for on-wiki and the Wikitech-ambassadors mailing list.

Event Timeline

Change 306560 had a related patch set uploaded (by Mooeypoo):
Append mw.notify div to the body so it appears on top of everything

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

Posting my answer to the discussion in the commit:

Regarding the first point - in vector, it works as intended, but in monobook, the overlay is on top of the personal toolbar now. I can change this with a style change to monobook (and it shouldn't be a bad idea to let skins decide where to pop that notice out, I think) - but that does raise the question of whether this is something that should be forced onto the skins to deal with or not, and it definitely make point #2 more important.
The main issue that raised the need for this here is MobileFrontend's usage of mw.notify. We wanted to add an mw.notify popup confirming that "x notifications were marked as read" when a user clicks the "mark all read" button in the overlay. This doesn't work if the mw.notify div is inside the content but the MobileFrontend overlay is outside it.
So the question now is what is more proper and what is better -- changing mw.notify to always be on top (in my opinion seems like a good behavior for global notification popup) or leave mw.notify as-is and claim this is a specific issue with MobileFrontend's use of it (or assume any other extension/tool should solve for this individualy)

@Esanders pointed out an idea that would fix both issues:

mw.notify can create two sections to overlay the popup - the default would be in the content (like it is now) but another would be "global" overlay in the body as a redundancy. When we call mw.notify() we can explicitly state to use the global one when needed.

That would solve all issues, it seems, and be "backwards compatible" for skins and whatever extensions or gadgets that use the current behavior.

You should give the global overlay an additional class so it can be positioned differently if required.

So this sounds like a good idea, and it should work - but the way that mw.notify is written right now, there are a lot of assumptions that all notifications are in the same known $area block. Setting up two area blocks, one in content and one global would require a significant change to the way things work.

I can definitely tackle this, but this will no longer be a trivial small change to the code.

Change 306560 had a related patch set uploaded (by Mooeypoo):
Add an option for global overlay in mw.notify div

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

Volker_E triaged this task as Medium priority.Aug 18 2017, 9:37 PM

Change 306560 merged by jenkins-bot:
[mediawiki/core@master] Allow skins to place notification container for mw.notify

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

Volker_E raised the priority of this task from Medium to Needs Triage.Nov 27 2018, 4:39 AM
Volker_E moved this task from Backlog to Done on the UI-Standardization-Kanban board.