Watchlist Expiry: Watchstar popup not always usable on VisualEditor
Closed, ResolvedPublicBUG REPORT
Actions

Assigned To

Authored By

	dom_walden
	Aug 14 2020, 1:03 PM

Description

As a Watchlist Expiry user, I want the watchlist pop-up to be accessible when I open VE, so that I can choose my desired watch period.

Background:

While editing with VisualEditor, if I click the watchstar the popup cannot always be interacted with. As a result, for example, you cannot choose an expiry value. One way to reproduce this is to click the watchstar while VE is still loading. However, there might be other circumstances in which this happens.

Acceptance Criteria:

Fix the issue, so that the pop-up is displayed upon clicking the star when editing in VE

Steps to reproduce problem:

Go to any page
Edit using VisualEditor
While the loading bar is still loading, click the watchstar

Expected behavior: Popup appears and you can select an expiry
Observed behavior: Popup appears faded and cannot be interacted with (note, even if I click the watchstar again after the page has loaded the popup is still not interactive)

Environment

Wiki(s): https://en.wikipedia.beta.wmflabs.org MediaWiki 1.36.0-alpha (d560bee) 2020-08-14T09:17:05

Screenshots (if applicable):

ve_noninteractable.png (295×658 px, 19 KB)

Screen Shot 2020-08-14 at 5.31.52 PM.png (436×925 px, 184 KB)

Details

Subject	Repo	Branch	Lines +/-
Move notification area to end of body	mediawiki/core	master	+9 -4
Move notification overlay outside $content	mediawiki/core	master	+20 -5
Move notification overlay outside $content	mediawiki/core	master	+17 -4

Customize query in gerrit

Related Objects

Mentioned In: T260262: Watchlist Expiry: popup is blocked by other elements in Monobook
T260338: Sticky header: Notification popup (mw.notify) position incorrectly in Vector 2
T260413: Watchlist Expiry: Popup expiry dropdown cut off in Visual Editor
Mentioned Here: T261430: Watchlist Expiry: tab to dropdown after tab on watchstar
T143837: Append mw.notify div to the body instead of content so it is always on top
rMWd560bee54ee5: Merge "WatchAction: avoid unnecessary UPDATEs when expiry is unchanged"

Event Timeline

dom_walden created this task.Aug 14 2020, 1:03 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 14 2020, 1:03 PM

ifried moved this task from New & TBD Tickets to Needs Discussion on the Community-Tech board.Aug 14 2020, 2:37 PM

ifried updated the task description. (Show Details)Aug 14 2020, 10:36 PM

MusikAnimal added a project: VisualEditor.Aug 15 2020, 12:37 AM

dom_walden updated the task description. (Show Details)Aug 17 2020, 8:37 AM

Note: This is existing behavior on production (see screenshot example below from English Wikipedia). However, the situation is worsened with the introduction of watchlist expiry, since the user is prevented from making a selection.

Screen Shot 2020-08-18 at 3.07.54 PM.png (250×904 px, 76 KB)

I looked briefly into this and what's happening is that VE is appending div#notification-area into its container because the notification was injected into div#content before VE finished initializing; this is why also subsequent notifications also looked grayed out and can't be interacted with unless you refresh the page.

@Esanders does this sounds about right?

dmaza mentioned this in T260413: Watchlist Expiry: Popup expiry dropdown cut off in Visual Editor.Aug 18 2020, 8:27 PM

Note this was a preexisting problem with VE and notifications, it's just a bigger problem now because there's a form user's can't interact with. We think something like https://gerrit.wikimedia.org/r/c/mediawiki/core/+/620145 might fix it since that places the notification overlay on the <body>.

ifried updated the task description. (Show Details)Aug 20 2020, 10:50 PM

Change 620145 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/core@master] Move notification overlay outside mw-body

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

gerritbot added a project: Patch-For-Review.Aug 21 2020, 12:36 AM

Samwilson claimed this task.Aug 21 2020, 12:36 AM

Samwilson edited projects, added Community-Tech (Kanban-2020-21-Q1); removed Community-Tech.

Samwilson moved this task from Ready 🎬 to Review/Feedback 💬 on the Community-Tech (Kanban-2020-21-Q1) board.

Moving the notification container is a pretty big change and was discussed at length before in T143837 (cc @Mooeypoo, @Krinkle). I would read over that ticket to make sure you aren't creating any of the same issues that were found there.

Make sure you test it against Monobook and Minerva and every possible overlay you can find, not just the VE loading overlay, e.g. OOUI dialog overlay, MultimediaViewer, jQuery UI dialogs, Echo, site search autocomplete...

Also check that you aren't changing any inherited styles, like font size.

Samwilson moved this task from Review/Feedback 💬 to In Development 💻 on the Community-Tech (Kanban-2020-21-Q1) board.Aug 25 2020, 2:24 AM

Thanks for the pointers, @Esanders.

I think it's still worth pursuing moving the notification area, but how about to above the $content instead of at the end of the document? That means it keeps its initial top offset so all calculations are the same as the current situation. I've updated the above patch to do that.

Some notes:

If a skin doesn't provide its own mw-notification-area, we add one and also wrap it in an overlay. This means that sometimes there's no overlay.
Not many skins provide their own notification area. Only MinervaNeue so far, according to codesearch.
The ones that do don't use this feature as a way to position the notifications, but rather just as a way to get it out of the content area. Minerva is like this, and moves it to the bottom of the document.
A few (e.g. DarkVector, Femiwiki) leave it where it is but tweak the top distance. This breaks if we move the area, but is a minor fix; this feels better than having the notification jumping around the place on scroll, as is the current behaviour).
Some skins change notification font sizes, borders etc., but usually just based on .mw-notification.area and not a more specific selector (Femiwiki is in the process of fixing this).
Some skins (e.g. Femiwiki, foreground) that have side margins that mean the notification area moves the page is scrolled down. This doesn't seem intentional, and would be fixed if we move the area.
The site search autocomplete results are not impacted (they're under the notifications).
No other overlays are broken, as far as I've been able to test.

Samwilson moved this task from In Development 💻 to Review/Feedback 💬 on the Community-Tech (Kanban-2020-21-Q1) board.Sep 7 2020, 3:07 AM

JTannerWMF moved this task from To Triage to Triaged on the VisualEditor board.Sep 10 2020, 4:34 PM

Change 620145 merged by jenkins-bot:
[mediawiki/core@master] Move notification overlay outside $content

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

When I run mw.notify('foo'); locally the message appears from a few milliseconds completely unstyled, causing the page to jump. The issue goes away when I turn of ResourceLoader debugging, which suggests that there is a race condition at play.

Video: https://www.youtube.com/watch?v=rlfjp6YiHmM&feature=youtu.be (play back at 0.25x speed to see the flicker clearly)

Esanders mentioned this in T260338: Sticky header: Notification popup (mw.notify) position incorrectly in Vector 2.Sep 10 2020, 6:53 PM

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.9; 2020-09-15).Sep 10 2020, 7:00 PM

In T260432#6451668, @Esanders wrote:

When I run mw.notify('foo'); locally the message appears from a few milliseconds completely unstyled, causing the page to jump. The issue goes away when I turn of ResourceLoader debugging, which suggests that there is a race condition at play.

Video: https://www.youtube.com/watch?v=rlfjp6YiHmM&feature=youtu.be (play back at 0.25x speed to see the flicker clearly)

It doesn't happen if the notification is triggered by the application (click on the watchstar or after saving preferences) even with RL debugging on. I wonder what the difference is.

Edit: Nevermind, it does if I throttle

ARamirez_WMF moved this task from Review/Feedback 💬 to QA 🐛 on the Community-Tech (Kanban-2020-21-Q1) board.Sep 10 2020, 10:06 PM

This change appears to move the popup down on "new" Vector and Modern skins, which exacerbates problems of the popup being cut off which already existed.

In some cases the popup is completely hidden.

This happens with watchlist expiry enabled and disabled.

You should be able to see this on https://en.wikipedia.beta.wmflabs.org.

This also affects other notifications (not just the watch star popup).

New Vector before this change:

After this change:

FYI, for new Vector, I only see the problem of the popup being cut off when I have a particular config.:

$wgVectorLayoutMaxWidth = true;
$wgVectorIsSearchInHeader = true;

In T260432#6451668, @Esanders wrote:

When I run mw.notify('foo'); locally the message appears from a few milliseconds completely unstyled, causing the page to jump. The issue goes away when I turn of ResourceLoader debugging, which suggests that there is a race condition at play.

Video: https://www.youtube.com/watch?v=rlfjp6YiHmM&feature=youtu.be (play back at 0.25x speed to see the flicker clearly)

I checked out a version prior to https://gerrit.wikimedia.org/r/620145 and this was already a problem. If I'm reading the code correctly mediawiki.notification module is lazy loaded on mw.notify and styles are not yet available when the popup content is added to the dom, and that's why throttling makes the issue more visible.

I'll create a new task for this but it is not a priority for us considering that it is currently not a problem in production or when RL debugging is off.

Change 626783 had a related patch set uploaded (by HMonroy; owner: HMonroy):
[mediawiki/core@master] Move notification overlay outside $content

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

Change 626918 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/core@master] Move notification area to end of body

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

Samwilson moved this task from QA 🐛 to Review/Feedback 💬 on the Community-Tech (Kanban-2020-21-Q1) board.Sep 14 2020, 8:21 AM

Change 626783 abandoned by HMonroy:
[mediawiki/core@master] Move notification overlay outside $content

Reason:
A new solution has been developed in patch: https://gerrit.wikimedia.org/r/c/mediawiki/core/ /626918

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

This change appears to move the popup down on "new" Vector and Modern skins, which exacerbates problems of the popup being cut off which already existed.

With new Vector there seems to be an existing problem with the switch between a floating and top notification (see vid below); this is fixed with the above patch to core and without any patch to Vector.

simplescreenrecorder-2020-09-15_13.23.53.mp4321 KBDownload

(Also, $wgVectorLayoutMaxWidth has been removed and is now the default. I got a bit confused this morning. :) )

The notification is no longer cut off, e.g.:

New Vector	Old Vector	MonoBook

In new Vector the notification is hanging out over the grey side area — I'm working on a fix for that, I'm not sure how much in flux new Vector is at the moment (@Jdlrobson are you someone to weigh in on this? And the notification-outside-$content thing in general?).

Change 626918 merged by jenkins-bot:
[mediawiki/core@master] Move notification area to end of body

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

ReleaseTaggerBot edited projects, added MW-1.36-notes (1.36.0-wmf.10; 2020-09-22); removed MW-1.36-notes (1.36.0-wmf.9; 2020-09-15).Sep 16 2020, 5:00 AM

Samwilson moved this task from Review/Feedback 💬 to QA 🐛 on the Community-Tech (Kanban-2020-21-Q1) board.Sep 16 2020, 6:08 AM

I cannot reproduce this bug anymore.

I experimented with different timings of watching and loading VE (watching before opening VE and at different stages of the VE loading process).

I attempted on each of the 6 skins (Vector new and legacy, Minerva, Modern, MonoBook and Timeless) + mobile (not that I think it makes a difference). In Timeless, Minerva and Mobile you cannot watch while in VisualEditor, but I tested clicking the watch star first and then loading VE while the popup was visible (this is how you could previously reproduce the bug in Timeless).

This change appends the notification area to the end of the <body> of the HTML, unless the skin specifically defines an area for notifications (which Minerva/Mobile does, hence why it has a different placement). I notice that there are a couple of other things which also get appended to the same location, including the VE overlay and the Echo notification area. You can change the order in which they are appended depending on which you activate first. From experimenting, changing the order did not seem to make a difference in behaviour (e.g. in terms of functioning, overlapping, wrapping on small screens) It might affect tab ordering, but that might be dealt with in T261430.

Make sure you test it against Monobook and Minerva and every possible overlay you can find, not just the VE loading overlay, e.g. OOUI dialog overlay, MultimediaViewer, jQuery UI dialogs, Echo, site search autocomplete...

I tested a few different overlays/UI widgets:

VisualEditors UI widgets and overlays (e.g. options, publish changes)
Search autocomplete
Echo notifications
Special:RecentChanges UI widgets
TagMultiSelect widget
StructuredDiscussions widgets

(Testing mainly on new Vector.)

The watch popup and the other overlays did not seem to interfere. The watch popup always appears "on top" and is always usable, although sometimes it does hide other overlays (temporarily).

Testing on an iPad-sized screen did not seem to make a difference.

Also tested some overlays on Mobile:

Search autocomplete
Echo notifications
Dropdown widgets (e.g. "More actions")
Multimediaviewer (e.g. https://en.wikipedia.beta.wmflabs.org/wiki/File:Ice_and_Snow_World-Sculpture_Festival_8_NOTCOMMONS.webm)

Because we are make a reasonably significant change to notifications, I tested a few existing notifications, including:

Page patrolled notification on legacy Vector, Minerva, MonoBook, Mobile
Copy to clipboard notification on MonoBook and Minerva (on Special:ApiSandbox)
Notification when saving Special:Preferences
Notification when adding categories to a page on Mobile

Where I could compare to testwiki, I saw no differences in appearance or behaviour.

Test Environment: Various different versions of https://en.wikipedia.beta.wmflabs.org.

Test Devices: Mainly Firefox, but also a couple of different mobile devices including Android and iOS.

dom_walden mentioned this in T260262: Watchlist Expiry: popup is blocked by other elements in Monobook.Sep 21 2020, 2:19 PM

I have tested this on https://en.wikipedia.beta.wmflabs.org, and the bug is no longer reproducible. If I click on the star icon while opening VE, the pop-up is not hidden or inaccessible. It was always accessible in my tests. For this reason, I'm marking this work as Done.

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptSep 21 2020, 11:49 PM

	F32351211: simplescreenrecorder-2020-09-15_13.23.53.mp4
	Sep 15 2020, 6:04 AM

	F32187861: Screen Shot 2020-08-18 at 3.07.54 PM.png
	Aug 18 2020, 7:08 PM

	F32144135: Screen Shot 2020-08-14 at 5.31.52 PM.png
	Aug 14 2020, 10:36 PM

Watchlist Expiry: Watchstar popup not always usable on VisualEditorClosed, ResolvedPublicBUG REPORTActions