Page MenuHomePhabricator

[Bug] toast messages not visible in overlays - errors caused by talk overlay are not visible
Closed, ResolvedPublic5 Story Points

Description

Currently when an error is thrown by the talk overlay it is displayed as a "toast error"

Given toasts are currently positioned below the overlay, these errors are not visible to the user so the error is not visible and the editor has no idea there has been a problem.

Developer notes

If I manually make the overlay opaque (for debugging purposes only) I can see that the toast is present but hidden under the overlay:

Currently the mw-notification-area is inserted inside #content
If this area is instead added as a child of the body element this seems to work just as well.

Acceptance criteria

  • Make sure the mw-notification-area element appears above overlays either via z-index or adjustments to the DOM

Testing criteria

  1. Visit https://reading-web-staging.wmflabs.org/wiki/Sandwich
  2. Login
  3. Open talk overlay and click "I like sandwiches"
  4. Drop internet connection
  5. Type yes in reply box and hit save
  6. Confirm you see a toast message above the talk overlay saying the error failed to save.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 21 2018, 10:54 PM
ovasileva triaged this task as Normal priority.Aug 22 2018, 8:52 AM
ovasileva added a subscriber: ovasileva.

@Jdlrobson - I'm not sure I'm following entirely here. What is the opaqueness for?

Jdlrobson updated the task description. (Show Details)Aug 22 2018, 2:55 PM
Jdlrobson renamed this task from [Bug] Errors caused by talk overlay are not visible to [Bug] toast messages not visible in overlays - errors caused by talk overlay are not visible.Aug 28 2018, 3:09 PM
Jdlrobson moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.
Jdlrobson updated the task description. (Show Details)Aug 28 2018, 4:24 PM
ovasileva set the point value for this task to 5.Aug 28 2018, 4:29 PM

We could update the code in core to look for an existing #mw-notification-area before creating a new one - this would give a skin the option to set a designated container.

Is this the cause of T202620?

Change 456665 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/skins/MinervaNeue@master] Add a notifications overlay container to the skin

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

Yup both of those patches are dupes!

Pulling into sprint for visibility and for us to assist with code review!

Change 306560 had a related patch set uploaded (by Jdlrobson; owner: Mooeypoo):
[mediawiki/core@master] Allow skins to place notification container for mw.notify

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

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

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

Change 456665 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Add a notifications overlay container to the skin

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

Ryasmeen reassigned this task from Ryasmeen to ABorbaWMF.Sep 12 2018, 6:11 PM
Ryasmeen added a subscriber: Ryasmeen.

Looks good to me across a few devices on Staging.

Jdlrobson reassigned this task from ABorbaWMF to ovasileva.Sep 12 2018, 10:05 PM
Jdlrobson added a subscriber: ABorbaWMF.
ovasileva closed this task as Resolved.Sep 13 2018, 2:24 PM

toasts are now there!

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptSep 13 2018, 2:24 PM
ovasileva updated the task description. (Show Details)Sep 13 2018, 2:25 PM