Page MenuHomePhabricator

"Other review tools" is not working when using Minerva on Recent Changes or JS is disabled on desktop
Open, Needs TriagePublic

Description

It is not possible to close the "Other Review tools" on https://en.m.wikipedia.org/wiki/Special:RecentChanges or when JS is disabled.

Tested on Chromium and Firefox, last versions on Ubuntu 16.04.

Same bug report from @Volker_E

Other review tools expand/collapse functionality is disabled in MobileFrontend.
Compare
https://en.m.wikipedia.org/wiki/Special:RecentChanges?hidebots=1&hidecategorization=1&hideWikibase=1&limit=50&days=7&enhanced=1&damaging__likelybad_color=c4&damaging__verylikelybad_color=c5&useskin=minerva&urlversion=2

versus
https://en.wikipedia.org/wiki/Special:RecentChanges?hidebots=1&hidecategorization=1&hideWikibase=1&limit=50&days=7&enhanced=1&damaging__likelybad_color=c4&damaging__verylikelybad_color=c5&useskin=minerva&urlversion=2

Apart from JS performance questions, the interface representation should be amended.

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterDisable RCFilters in mobile view
mediawiki/core : masterChangesListSpecialPage: Add hook to disable structured filters UI

Event Timeline

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptNov 28 2017, 6:43 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jdlrobson removed a project: MinervaNeue.EditedDec 7 2017, 9:02 PM
Jdlrobson added a subscriber: Jdlrobson.

:-(

This is not a problem with Minerva. It works perfectly well on desktop:
https://en.wikipedia.org/wiki/Special:RecentChanges?useskin=minerva
I suspect some code is not being loaded with 'targets'=>'mobile' that should be.
It's also broken on "mobile vector"

Hi Jon. I'm sure this is a different issue from T178194, but I don't really understand the distinction. Can you please explain?

Jdlrobson added a comment.EditedDec 11 2017, 6:35 PM

As reported by @Trizek-WMF, a button is being rendered that when clicked does nothing. This is very visibly broken and in a prominent place in the UI.
It's one thing to not enable the new RC feature on mobile (T178194) but another to impact user experience by adding a button that doesn't work. This is a bug that should be fixed.

jmatazzoni added a comment.EditedDec 11 2017, 8:46 PM

I understand that what I'm seeing is not right. I just don't understand what I'm seeing. The "other review tools" is part of the New Filters for Edit Review UX. But the tools on the version you're looking at are not the New Filters tools. So what's wrong here is not precisely that you can't use that link to close the box and hide the links. It's that you're seeing the Other Review Tools link at all. In other words, you're seeing a mix of the new UX and the old UX.

As I say, I don't exactly understand what you're trying to do or what this version is I'm looking at. I'm looking at Minerva on mobile here, and it works fine

So when you show this link,

is that Minerva or is it just mobile? (My phone isn't working right now so I can't use it to look).

Mobile view does not have different skins - so Minerva skin is a desktop skin. Btw, "Other Review tools" link work with Minerva skin (on a mobile device too).

:-(
This is not a problem with Minerva. It works perfectly well on desktop:
https://en.wikipedia.org/wiki/Special:RecentChanges?useskin=minerva
I suspect some code is not being loaded with 'targets'=>'mobile' that should be.

You're right. RCFilters is trying to load, and believes that it's loading (you can see the rcfilters-enabled class on the <body>, for example), but the relevant modules don't have 'target' => [ 'desktop', 'mobile' ] on them, so they don't load.

We have two options here: either we change the target setting, which will cause the new UI to load on mobile, or we make the server realize that the new UI is not going to load on mobile and stop trying. Long term, the former would be nice, but short term I think we should do the latter, because of how broken the new UI is on mobile right now.

In T181545#3835433, @Catrope wrote:

...We have two options here: either we change the target setting, which will cause the new UI to load on mobile, or we make the server realize that the new UI is not going to load on mobile and stop trying. Long term, the former would be nice, but short term I think we should do the latter, because of how broken the new UI is on mobile right now.

I agree. We specifically blocked the new UI from mobile because we aren't able to commit the resources at this point to optimize it there—though there have been requests for that (T178194). We should evaluate this as a project at some point in the near future.

I'm having trouble figuring out how to disable RCF on mobile effectively, since the question of whether we are in the mobile view or the desktop view is MobileFrontend-specific, and this code is in core. @Jdlrobson , maybe you could help here?

However, I noticed this is equally broken in no-JS mode. The button is there and is not interactive, but the content it's toggling is always visible. So I think we should instead try to hide the button in no-JS mode and mobile mode (maybe by hiding it by default but showing it in CSS inside a module that has target=desktop).

Change 398200 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/core@master] ChangesListSpecialPage: Add hook to disable structured filters UI

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

Change 398202 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/extensions/MobileFrontend@master] Disable RCFilters in mobile view

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

I ended up proposing a new hook so that MobileFrontend can disable RCFilters for mobile views.

However, I noticed this is equally broken in no-JS mode. The button is there and is not interactive, but the content it's toggling is always visible.

This is probably the real issue here. It's okay to show the icon in mobile mode but if it requires JS and that code has not loaded it should be dealt with.

I ended up proposing a new hook so that MobileFrontend can disable RCFilters for mobile views.

Are there any alternatives to doing it this way? For instance, could we make this work without JS or be hidden if JS is not loaded? I usually advise the disable via hook approach only as a last resort.

I ended up proposing a new hook so that MobileFrontend can disable RCFilters for mobile views.

Are there any alternatives to doing it this way?

We could look at @Catrope's "So I think we should instead try to hide the button in no-JS mode and mobile mode (maybe by hiding it by default but showing it in CSS inside a module that has target=desktop)." idea instead of the hook.

But ideally if we do that, we should move mw-rcfilters-enabled and mw-rcfilters-disabled to JS (for consistency).

I think doing it server-side was to avoid a potential FUOC with gadgets using the classes, but I don't remember (probably not) if I actually verified such as a FUOC.

Catrope added a comment.EditedDec 21 2017, 12:30 PM

There is a CSS alternative, but it's kind of ugly. We could have MobileFrontend add a body class (this is now easy with $output->addBodyClasses( [ 'foobar' ] );) like mw-mf-body or something, to flag page views that are MF page views. Then we could apply the same CSS to this component when wrapped in .mw-mf-body as when it's wrapped in .client-nojs: hide the button and make the content visible.

I actually went all the way through implementing this, but the problem I ran into was finding a good place to put that CSS. I don't want to put it with the rest of the RCFilters CSS, because it lives in MW core and I don't want to reference MF-specific selectors in there. I also didn't really want to put something this specific in MF, although I guess we could, and we'd have to worry about keeping the MF RCF CSS in sync with the core RCF CSS.

If you have ideas for solutions/compromises for this, I'd be happy to hear them. I didn't find anything satisfying myself, and the longer I looked at it the more I felt that the confluence of circumstances that causes RCF to be "disabled" on mobile while not really being disabled (the server generates HTML as if it's enabled, and the .mw-rcfilters-enabled class is set so gadgets and user scripts think it's enabled too) was fragile and problematic, and that it would be simpler and safer to disable RCF server-side when in mobile. There isn't a great way for RCF (which is in core) to check if we're in mobile mode and disable itself, because that code lives in MF, so I added a hook because that's what we normally do when we have a core->extension reverse dependency problem. Another approach I considered is checking if $output->getTarget() === 'mobile', I forget if I got that to work or not.

Jdlrobson added a comment.EditedJan 3 2018, 10:42 PM

I think given it's broken on desktop with JS disabled as well we should fix this in core. I'm not sure why MobileFrontend needs to add a body class. or add CSS.

Why not just add

.client-nojs .mw-recentchanges-toplinks-title,
.client-nojs .mw-recentchanges-toplinks-content {
    display: none;
}

(Alternatively use CSS :active selectors to make it work without JS)

What am I missing?

Jdlrobson renamed this task from "Other review tools" is not working when using Minerva on Recent Changes to "Other review tools" is not working when using Minerva on Recent Changes or JS is disabled on desktop.Jan 3 2018, 10:42 PM
Jdlrobson updated the task description. (Show Details)

I think given it's broken on desktop with JS disabled as well we should fix this in core. I'm not sure why MobileFrontend needs to add a body class. or add CSS.
Why not just add

.client-nojs .mw-recentchanges-toplinks-title,
.client-nojs .mw-recentchanges-toplinks-content {
    display: none;
}

(Alternatively use CSS :active selectors to make it work without JS)
What am I missing?

Yes, you're right that we need to add that, to fix no-JS. I forgot about that in my confusion, and I'll put up a patch for that tomorrow. (However, we should not do what you suggest here; we should hide the button but NOT hide the content that it toggles, because that content is useful to no-JS users.)

However, that won't fix the case where you're running MF with JS enabled. The JS modules for RCF don't load (because they don't have target=mobile), but it's also not the case that JS is disabled, and so the client-nojs class isn't present. Figuring out how to detect that we're in JS-is-enabled-but-not-really-present mode is the hard part. I presented some solutions to it on this task earlier, and none of them are all that pretty.

(We could also have the RCF JS code add a CSS class when it runs, but that happens pretty late, so if we do that we cause a FOUC, and we just spent a bunch of time and effort eliminating FOUCs in RCF as much as we could.)

However, that won't fix the case where you're running MF with JS enabled.

That's true, but it's also dangerous to believe that JS will always load correctly. This should be resilient to dropped connections/JS errors.
Ideally, the button would progressively enhance an existing element that is harmless without JavaScript e.g. heading or a link to somewhere.
I think the real problem here is the rendering of a link with role button on the server side rather than adding the button functionality in JS where it is required.

I agree that this approach doesn't handle cases where JS doesn't load correctly, but I don't see how we can handle those without causing an FOUC. Fundamentally, we don't know whether the JS will load correctly until the page has already been visible to the user for some time, so I think we have a binary choice here between eliminating FOUCs for almost all users on the one hand, or fixing intermittent misbehavior for a small minority(*) on the other hand.

(*) I don't mean no-JS users, I mean (non-mobile) users for whom the JS fails to load

This problem seems very similar to the one we had with section collapsing on mobile where we didn't want a FOUC where sections were collapsed.

When you refer to FOUC I assume you mean reflows that would be associated with hiding/showing the button? If that's the case, I agree, but on the other hand I think it's perfectly fine for the arrow indicator to only show when the feature has been enabled. In fact it's misleading to show it before hand.

I don't know enough about OOUI but if you could reserve space and create/show the .oo-ui-indicatorElement-indicator hidden: invisibility with JS when you add the functionality for collapsing you will avoid a FOUC and the caret will appear when it is functional (currently if you click it before the JS loads it has no effect). Without the caret, the UI is less misleading - it just seems like a heading (in fact the use of an a tag for "other review tools" seems wrong to me - I'd actually make it a h2 or other heading.)

Using a hook/adding special handling for mobile is just going to bite you or some other poor developer later on as the target system eventually gets deprecated (T127268). This is why I usually discourage adding a module without targets mobile. Alternatively just load all the code on mobile (T178194) even though you know it's a broken experience. This is what Special:Translate does and at least that is relatively functional and usable albeit with inferior user experience. Of course that doesn't fix the page on broken connections... but at least then it's a shared (low priority?) problem across all platforms.

Restricted Application added a project: Growth-Team. · View Herald TranscriptJul 30 2018, 7:35 PM

Change 398202 abandoned by Jdlrobson:
Disable RCFilters in mobile view

Reason:
This patch is now 334 days old. Hopefully we'll be mobile-fying this page as part of the advanced mobile contributions project.

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

Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptOct 12 2019, 4:01 AM