Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Page MenuHomePhabricator

MobileFormatter doesn't run on complex pages and gives no feedback to editors creating confusion
Open, Stalled, MediumPublic

Description

On this page: https://en.m.wikipedia.org/wiki/List_of_works_by_Vincent_van_Gogh top-level headings are not shown as a collapsible section on mobile.

This happens because there are too many images in the page., but the editor is given no feedback to why this has happened. When an edit happens that causes this, the editor should be notified that they may be reducing the mobile friendliness of the page with actionables on how to fix it (reduce the number of images for example).

See the discussion on enwiki Village Pump for background.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Aklapper added a subscriber: RhinosF1.

If this only happens on mobile then this might be in the MobileFrontend extension itself instead of MediaWiki (or maybe maybe in the MinervaNeue skin).

This diff also has the issue meaning the issue is likely caused by the number of files or the size of the file links.

It's not just Minerva, the dropdown icon doesn't show when Vector is set as the mobile skin as well, so it's a MobileFrontend issue.

MobileFrontend sets MFMobileFormatterOptions.maxImages to 1000, and since there's over 1000 images the mobile formatting is not being applied.

MobileFrontend sets MFMobileFormatterOptions.maxImages to 1000, and since there's over 1000 images the mobile formatting is not being applied.

@Jdlrobson wrote the patch that introduced the maxImages and maxHeadings settings and how they're applied (see I877473bbeb8410f4959553076a78f7c51e4b473e) so his input and any context he can give us would be welcome.

However, it seems to me that those settings should only apply to their respective transforms rather than to all transforms, i.e.

includes/MobileFormatter.php
public static function canApply( $text, $options, $transformName ) {
  switch ( $transformName ) {
    case 'HEADINGS':
      return preg_match_all( '/<[hH][1-6]/', $text ) < $options['maxHeadings'];
    case 'IMAGES':
      return preg_match_all( '/<img/', $text ) < $options['maxImages'];
    default:
      return true;
  }
}

For example, I would hope that the MobileFormatter::makeSections transform would run for a page has 1000+ images but only 6 sections.

phuedx triaged this task as Medium priority.Mar 31 2020, 3:45 PM

Prioritised as Medium to match the priority of the task associated with the original change.

phuedx renamed this task from h2 doesn't get tagged as collapsible-heading on mobile to Sections aren't collapsible when there are many images on the page.Mar 31 2020, 3:52 PM

The MobileFormatter cannot handle large pages with lots of DOM nodes. That's why this restriction is in place. The problem is the code that parses the code into a DOM. If we can't do that we can't run any transformations including images or headings.

My suggestion for now would be to give feedback in the editor via a warning message that the page is too complex for mobile formatting.

Jdlrobson renamed this task from Sections aren't collapsible when there are many images on the page to MobileFormatter doesn't run on complex pages and gives no feedback to editors creating confusion.Mar 31 2020, 4:27 PM
Jdlrobson added a project: Web-Team-Backlog.

(We can also try upping those values as they were set arbitrarily based on articles which were experiencing timeout values. I believe @nray did that analysis.)

At what level is the transformation done? If the large page were broken up into sufficiently small subpages which were then transcluded into a composite top-level page, would that solve the problem?

@RoySmith it's done on the entire finished page so transclusions wouldn't help here. Sorry to not bne able to provide any quick fixes for now :( If you are looking for a short term fix using flag emojis rather than images should help. https://flagpedia.net/emoji

Interesting thought, thanks. I'll suggest it to the OP.

It turns out the OP is having problems accessing phab, but they implemented something very similar and that did indeed resolve the issue.

Jdlrobson edited projects, added MobileFrontend (Tracking); removed MobileFrontend.
Jdlrobson moved this task from Tracking to Team: web on the MobileFrontend board.
Jdlrobson edited projects, added MobileFrontend; removed MobileFrontend (Tracking).

Change 671000 had a related patch set uploaded (by BrandonXLF; owner: BrandonXLF):
[mediawiki/extensions/MobileFrontend@master] Add comment when MobileFormatter can't be applied

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

@RoySmith would an HTML comment that doesnt show up in the editor be sufficient here?

@BrandonXLF perhaps a <div class="error"></div> is best here as this can be considered a parsing errors like the ones we're discussing in T280766

Jdlrobson changed the task status from Open to Stalled.Jul 15 2021, 10:57 PM

We are waiting on a reply from @RoySmith for https://phabricator.wikimedia.org/T248796#6915997 to understand this request a little better.

@Jdlrobson I'm sorry, I didn't realize this was waiting on me for a response. I'm not entirely sure what to tell you. My involvement here was mostly noticing somebody else describing the problem on Village Pump and doing some initial troubleshooting.

As I understand it, this is a "page is too complex" limitation in the MobileFrontend extension which is going to be difficult or impossible to fix, so what we're talking about is how best to communicate that to the user. And the fix suggested in change 671000 is to add an HTML comment which includes the error message.

To be honest, I don't think that's going to be terribly useful. I'm sure somebody like the person who originally reported this on Village Pump would never see that. I doubt I would have seen it either (I'm a software engineer who knows enough to poke around in a browser console but is unfamiliar with the MediaWiki codebase). There's a ton of HTML on a typical wiki page. The odds that I would notice an embedded comment are slim to none.

But, here's a thought; had there been something in the javascript console log, I probably would have noticed it. So maybe instead of (or perhaps in addition to) an embedded HTML comment, a bit of embedded javascript which logs the error to the console? The volume of stuff in a typical console log is quite limited, so it would be much more likely to be noticed there.

I'm not sure how

My suggestion for now would be to give feedback in the editor via a warning message that the page is too complex for mobile formatting.

would look to the editor, but it sounds like a good plan in general.

I hope that addresses your question.

Jdlrobson changed the task status from Stalled to Open.Jul 15 2021, 11:53 PM

Thanks that's very clear and does address my question. @BrandonXLF I left a comment on the patchset which I hope helps us resolve this one!

If I or some other person were interested in finding pages with this issue, either to simplify them or to quantify their prevalence, how might that might be done? Is there any global view of the issue? The patch mentioned above (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/671000/) just places an (perhaps not very obvious) indication on the page. Could pages with this issue (when MobileFormatter can't be applied) be added to a list in Special:SpecialPages or be made publicly searchable?

Could pages with this issue (when MobileFormatter can't be applied) be added to a list in Special:SpecialPages or be made publicly searchable?

That doesn't seem to be possible, since it's decided based on the HTML of the rendered page, when it's serving it to users, and not when the page is being parsed. At this stage, it can't write to database (for performance reasons).

It would be different if MobileFrontend would store a page property for this at parsing time. I don't know why it wasn't done at this stage, since the recommendation at T232690#5486265 was to use ParserOutput to avoid a preg_match_all. Of course, that would require expensive pages to be parsed again to store those properties on the database.

A conversation on the enwiki village pump (technical) is ongoing, and I'd like to relay a suggestion made there: If possible, pages with this problem could be automatically placed in a tracking category. This would be an item at Special:TrackingCategories, and is apparently the typical way to keep tabs on such issues. I apologize if this is technically difficult.

Yeh a tracking category is not currently possible as this runs /after/ parsing has occurred.

One thing we could do is log an error that could be seen in logstash, but given most editors don't have access to that, someone would need to find a way to relay that information to bots and/or relevant communities.

That said, the logic here is really simple.

https://gerrit.wikimedia.org/g/mediawiki/extensions/MobileFrontend/+/5a3058552ed43521df448f2edf6653bbe6eb7b0d/includes/MobileFormatter.php#95
https://gerrit.wikimedia.org/g/mediawiki/extensions/MobileFrontend/+/5a3058552ed43521df448f2edf6653bbe6eb7b0d/extension.json#788

It happens when an article has more than 1000 images and/or 4000 headings.
Is there a way this can be detected on edit?

A gadget could be crafted that added the category automatically like so:

if(
  ( $('.mw-body-content img').length> 1000 || 
  $('.mw-body-content').find('h1,h2,h3,h4,h5').length > 4000  ) &&
mw.config.get('wgCategories',[]).indexOf('Category:Not compatible with MobileFormatter') === -1 ) {
  
 alert('This page will not work on T248796. Auto-adding category');
// @todo: add category to page.
}

I have added a "compensation" for mobile users into enwiki [[List of works by Vincent van Gogh]]: a multiple image is added to allow the mobile user, reading without TOC, at least to jump to the different lists.

I wasn't aware of the fact that mobile formatter was disabled for large pages before I saw @Ruedi33a's report. It seemed very sad that it's disabled for exactly the pages that need it most.

So I had a look, and it turns out that the performance problem was easy to fix: see details in T317070: MobileFormatter has quadratic performance. If those changes are approved, then the formatter will just work on all pages, and this becomes a moot point!

Given the promising developments in T317070 it seems like we can avoid this entirely by removing the constraint altogether.

My attempts to remove the constraint was actually reverted back in September (T317070#8274404). I only learned about this now. This is still a problem.

Jdlrobson changed the task status from Open to Stalled.Mar 17 2023, 6:00 PM

Web team will track blocking work in T318991 so this is stalled until that's happened.

Change #671000 abandoned by Jdlrobson:

[mediawiki/extensions/MobileFrontend@master] Add comment when MobileFormatter can't be applied

Reason:

Hello this is an automated message.
I am abandoning this patch as it over a year old, and is not currently in a mergeable state. This has nothing to do with the quality of the patch.

If you still care about this patch, please feel free to restore it and rebase it, and we can happily continue the conversation to help you get it merged.

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