Page MenuHomePhabricator

Can't click notifications in Minerva desktop
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

Screen Shot 2022-07-22 at 10.32.45 AM.png (117×1 px, 14 KB)

What happens?:
Nothing

What should have happened instead?:
It should open the overlay

Software version (skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

Event Timeline

Restricted Application added subscribers: Masumrezarock100, Aklapper. · View Herald Transcript
Tgr subscribed.

Confirming, I also ran into this a few days ago.

Note this is addressed in the larger refactor https://phabricator.wikimedia.org/T257143
I can't remember what specifically was the problem here but hopefully a more shorter term fix can be pulled out.

See also
https://phabricator.wikimedia.org/T310358
https://phabricator.wikimedia.org/T310559

An update on this based on conversations with Dennis just now coming out of some investigations Growth has done here:

  • Re: priority: there are a relatively small number of desktop users on Minerva
  • A Growth engineer took an initial stab at this but encountered difficulties in navigating the codebase
  • The thought for now from Growth is that this would make sense as a fix within Minerva for now, initially putting aside the broader refactor, though there is some wiggle room given that eventually this will live in Echo/Notifications

I can't remember which was the prior status quo, if the overlay or the dropdown should open in this case. My guess is the overlay. In any case, in ext.echo.init.js initialization code there's:

 $(function() {
  if (mw.config.get('wgMFMode')) {
    initMobile();
   } else {
    initDesktop();
   }
});

In the described scenario mw.config.get('wgMFMode') === null and initDesktop() is called failing to find and element with mw-echo-notification-badge-nojs class and not attaching any handler to it. Is the expected behavior to call initDesktop?

I can't remember which was the prior status quo, if the overlay or the dropdown should open in this case. My guess is the overlay. In any case, in ext.echo.init.js initialization code there's:

 $(function() {
  if (mw.config.get('wgMFMode')) {
    initMobile();
   } else {
    initDesktop();
   }
});

In the described scenario mw.config.get('wgMFMode') === null and initDesktop() is called failing to find and element with mw-echo-notification-badge-nojs class and not attaching any handler to it. Is the expected behavior to call initDesktop?

Yeah I think the overlay should open, it should function like other code that is targeting Minerva, it's just that this check is looking for wgMFMode and not necessarily if the skin is Minerva. Adding a condition to account for that should work, if a bit awkward.

Change 818213 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/Echo@master] Run mobile inititializtion code for Minerva skin

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

The use of desktop Echo was an intentional decision as Minerva is designed to work without MobileFrontend installed and Echo relies on MobileFrontend code. I believe this change would break that for third parties without MobileFrontend installed? In addition to this the slide out drawer experience is kind of odd on a desktop browser.

Change 821591 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] Hot fix: Make desktop Minerva icon clickable

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

@kostajh any interest in a hot fix here or should I abandon this patch? (also posted on Gerrit, but putting here for transparency to others following along)

@kostajh any interest in a hot fix here or should I abandon this patch? (also posted on Gerrit, but putting here for transparency to others following along)

Sorry to be late to respond. It seems to me that the hot fix might be adding more complexity than is worth, given that this is (I think) not a common use case. I'll follow up on the patch.

Change 818213 abandoned by Kosta Harlan:

[mediawiki/extensions/Echo@master] Run mobile inititializtion code for Minerva skin

Reason:

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

Change 821591 abandoned by Jdlrobson:

[mediawiki/extensions/Echo@master] Hot fix: Make desktop Minerva icon clickable

Reason:

I am going to revisit this after the changes we have planned for Vector. We're realizing we can't make a lot of changes we need for desktop improvements while https://phabricator.wikimedia.org/T257143 is open so going to try and make progress on that within the web team instead. I will keep you posted.

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

Change 821591 abandoned by Jdlrobson:

[mediawiki/extensions/Echo@master] Hot fix: Make desktop Minerva icon clickable

Reason:

I am going to revisit this after the changes we have planned for Vector. We're realizing we can't make a lot of changes we need for desktop improvements while https://phabricator.wikimedia.org/T257143 is open so going to try and make progress on that within the web team instead. I will keep you posted.

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

Moving off the current sprint board per the above comment.

Change 834614 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] WIP: Address issues with Minerva Echo icon

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

Change 867303 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] Make Echo icon clickable in desktop Minerva when more than 0 notifications

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

Change 834614 abandoned by Jdlrobson:

[mediawiki/skins/MinervaNeue@master] WIP: Address issues with Minerva Echo icon

Reason:

This has been split into https://gerrit.wikimedia.org/r/c/mediawiki/skins/MinervaNeue/+/867305 https://gerrit.wikimedia.org/r/c/mediawiki/skins/MinervaNeue/+/867304 and https://gerrit.wikimedia.org/r/c/mediawiki/skins/MinervaNeue/+/867303

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

Change 867304 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] Show both Echo icons in desktop Minerva

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

Change 867303 abandoned by Jdlrobson:

[mediawiki/skins/MinervaNeue@master] Make Echo icon clickable in desktop Minerva when more than 0 notifications

Reason:

See https://gerrit.wikimedia.org/r/c/mediawiki/skins/MinervaNeue/+/867304

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

Change 867304 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Show both Echo icons in desktop Minerva

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

Since this is desktop Minerva this can skip web team process. I've manually tested this on beta cluster and it LGTM.
https://en.wikipedia.beta.wmflabs.org/wiki/Albert_Einstein?useskin=minerva