"Other review tools" is not working when JS is disabled on desktop
Open, Needs TriagePublic
Actions

Assigned To

None

Authored By

	Trizek-WMF
	Nov 28 2017, 6:43 PM

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

	Subject	Repo	Branch	Lines +/-
	ChangesListSpecialPage: Add hook to disable structured filters UI	mediawiki/core	master	+14 -0
	Disable RCFilters in mobile view	mediawiki/extensions/MobileFrontend	master	+22 -0

Customize query in gerrit

Related Objects
Search...

		Status	Subtype	Assigned	Task
		Resolved	Goal	ovasileva	T198313 [GOAL] Advanced mobile contributions
		Open		None	T181545 "Other review tools" is not working when JS is disabled on desktop

Event Timeline

Trizek-WMF created this task.Nov 28 2017, 6:43 PM

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

Aklapper added a project: MinervaNeue.Nov 28 2017, 9:49 PM

:-(

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?

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.

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

https://en.m.wikipedia.org/wiki/Special:RecentChanges?useskin=minerva

So when you show this link,

https://en.m.wikipedia.org/wiki/Special:RecentChanges

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

• jmatazzoni edited projects, added Collaboration-Team-Triage (Collab-Team-This-Quarter); removed Collaboration-Team-Triage.Dec 12 2017, 8:28 PM

• jmatazzoni moved this task from Untriaged to Ready for Pickup on the Collaboration-Team-Triage (Collab-Team-This-Quarter) board.

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).

In T181545#3821359, @Jdlrobson wrote:

:-(

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

gerritbot added a project: Patch-For-Review.Dec 14 2017, 1:43 AM

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.

Catrope claimed this task.Dec 14 2017, 1:57 AM

Catrope moved this task from Ready for Pickup to Needs Review on the Collaboration-Team-Triage (Collab-Team-This-Quarter) board.

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.

In T181545#3850212, @Jdlrobson wrote:

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.

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.

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)

In T181545#3873296, @Jdlrobson wrote:
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.

Jdlrobson mentioned this in T183720: QuickEvents: Launch declarative event-to-module binding.Jan 10 2018, 7:22 PM

Kaartic subscribed.Jul 30 2018, 7:35 PM

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

• jmatazzoni unsubscribed.Jul 30 2018, 8:07 PM

JTannerWMF moved this task from Inbox to Needs Discussion on the Growth-Team board.Aug 9 2018, 6:24 PM

JTannerWMF moved this task from Needs Discussion to Triaged but Future on the Growth-Team board.

Jdlrobson merged a task: T204851: Special:RecentChanges “Other review tools“ JS disabled in MobileFrontend .Sep 19 2018, 5:32 PM

Jdlrobson updated the task description. (Show Details)

Jdlrobson added subscribers: Volker_E, • alexhollender_WMF.

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

Jdlrobson added a parent task: T198313: [GOAL] Advanced mobile contributions.Nov 13 2018, 10:56 PM

Jdlrobson added a project: Web-Team-Backlog (Tracking).

Aklapper removed a project: Collaboration-Team-Triage (Collab-Team-This-Quarter).Oct 12 2019, 4:01 AM

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

I think all that's needed here after AMC has completed is

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

What am I missing?

Trizek-WMF unsubscribed.Feb 4 2020, 2:32 PM

Change 398200 abandoned by Jdlrobson:
ChangesListSpecialPage: Add hook to disable structured filters UI

Reason:
I think this is not needed any more. See https://phabricator.wikimedia.org/T181545#5847460 Feel free to reopen if I'm wrong!

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

Maintenance_bot removed a project: Patch-For-Review.Feb 7 2020, 4:10 AM

Re-checked to see the current behavior. To summarize

The issue (not being able to close Other review tools) is present only if Javascript is disabled for a browser used. It doesn't matter which skin (or desktop/mobile) is used.

If "Use non-JavaScript interface" option is set in Special:Preferences-Recent changes, the issue is not present.

This task has been assigned to the same task owner for more than two years. Resetting task assignee due to inactivity, to decrease task cookie-licking and to get a slightly more realistic overview of plans. Please feel free to assign this task to yourself again if you still realistically work or plan to work on this task - it would be welcome!

For tips how to manage individual work in Phabricator (noisy notifications, lists of task, etc.), see https://phabricator.wikimedia.org/T228575#6237124 for available options.
(For the records, two emails were sent to assignee addresses before resetting assignees. See T228575 for more info and for potential feedback. Thanks!)