Page MenuHomePhabricator

[BUG] Thumb border images in articles shown are being shown with no border in the Android app
Open, NormalPublicBug

Description

Images that have been defined in articles as having a border should be shown with a border in the app.

Steps to reproduce
  1. Open the article for Flag of Japan in the Android app
  2. Expand the infobox to see the Flag of Japan image.
Expected

Flag of Japan should be shown with border (as per web and iOS app):

Actual

Image is shown without border

Occurring on

Wikipedia v2.7.239-r-2018-07-31

Event Timeline

eranroz created this task.Jun 30 2018, 3:57 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
RHo closed this task as Declined.Aug 20 2018, 4:24 PM
RHo added a subscriber: RHo.

The Android and iOS app both do not have a border on thumbnail images, this is as intended.

eranroz reopened this task as Open.Aug 20 2018, 4:40 PM

So I would like to open it for discussion - This is especially important for flags with white background, such as flag of Japan where itsn't clear where are the flag borders with border option.
Can you please refer to the discussion about it?

RHo added a comment.Aug 20 2018, 5:15 PM

hi @eranroz - as far as I am aware there has not been specific discussion, as both apps have not used a border for a while now on thumbnail images, nor on mobile web:

Thumbnail on mobile web
Thumbnail on Android search results

However, upon reading the task description more closely, can I just confirm whether you were instead referring to when images are shown within articles that have a border defined? In that case there does appear to be a bug on Android where the border attribute is not being respected when the image appears in a quick box:

Borders show as expected in images within the article
Bug - no border when in the infobox:

Please confirm if this is accurate, and I will move this to be fixed in Bugs. Thanks!

@RHo yes, this is about images within articles. Thanks

RHo renamed this task from thumbborder images shown with no border in android app to [BUG] Thumb border images in articles shown are being shown with no border in the Android app.Aug 20 2018, 6:23 PM
RHo updated the task description. (Show Details)
RHo moved this task from UX Debt Backlog to Bug Backlog on the Wikipedia-Android-App-Backlog board.
cmadeo added a subscriber: cmadeo.Nov 1 2018, 2:40 PM

@Sharvaniharan, the border color should be #c8ccd1 (eg. Base70). Thanks!

Sharvaniharan added a comment.EditedNov 1 2018, 5:32 PM

The root cause for this is, the class "thumbborder"[as is seen in web/mobile content] is being removed from the <img> tag.

Eg as on web content now :
<img alt="Flag of Japan.svg" src="upload.wikimedia.org/wikipedia/en/thumb/9/9e/Flag_of_Japan.svg/255px-Flag_of_Japan.svg.png" width="255" height="170" class="thumbborder" srcset="upload.wikimedia.org/wikipedia/en/thumb/9/9e/Flag_of_Japan.svg/383px-Flag_of_Japan.svg.png 1.5x, //upload.wikimedia.org/wikipedia/en/thumb/9/9e/Flag_of_Japan.svg/510px-Flag_of_Japan.svg.png 2x" data-file-width="900" data-file-height="600">

bearND added a subscriber: bearND.Nov 14 2018, 5:34 PM

The Parsoid HTML actually doesn't have this attribute to begin with. Instead it wraps the <a><img> into a <figure-inline class="mw-image-border">.

content.parsoid.less has a rule to make the border appear in the Parsoid version:

/* Same as img.thumbborder in content.css */
.mw-image-border > *:first-child {
	> img,
	> video {
		border: 1px solid #eaecf0;
	}
}

We might need to look into getting those rules added to the base CSS.

Nirzar added a subscriber: Nirzar.Nov 19 2018, 8:51 PM

@bearND are we proposing to duplicate only this class styles? or duplicating all the styles that are missing in Minerva? for later, we might want to see what elements will be impacted by this

@Nirzar I'm not sure what the best way to handle this is. This will affect web a little bit, too, at least by bloating up the CSS. So, I thought it's best to ask for advice first.

bearND added a subscriber: phuedx.Nov 20 2018, 5:38 PM

I think we should check what content.parsoid.less does to the content on minerva, and if it is acceptable (the file is quite small) then including that file/module to it would be a good idea, making mobile web content styles work with parsoid HTML for the future parser migration.

If it doesn't work well, we could have a minerva parsoid module where we add this kind of rules for styling parsoid content.

@phuedx can you route this to your team, to get some thoughts?

Jhernandez triaged this task as Normal priority.Jan 9 2019, 5:00 PM

We should be testing the content.parsoid.less styles on the mobile output and seeing if there is anything very different or broken, to evaluate if we should just include the base parsoid styles into the base apps CSS.

If the content.parsoid.less styles are too vector specific, another option can be fixing them to be less vector specific and moving that CSS to vector, so that it doesn't apply to minerva.

LGoto added a subscriber: JoeWalsh.Jan 9 2019, 5:02 PM

@Jdlrobson I think we're going to want the content.parsoid.less styles included in Minerva as well for this to properly work? Thoughts?

I think mediawiki.skinning.content.parsoid is what you want:
https://en.wikipedia.org/w/load.php?modules=mediawiki.skinning.content.parsoid&only=styles&useskin=minerva

That's what VE on mobile uses.

bearND claimed this task.EditedJan 17 2019, 4:59 PM
bearND added a subscriber: Sharvaniharan.

@Jdlrobson Yes, that's what I want. I had already included the module name mediawiki.skinning.content.parsoid in my previous request. Then I was now wondering why the .mw-image-border rules didn't get included in my version of the request.
After trying changing various query parameters to get them closer to your request I suspect that it was the useskin=minerva that triggers it. I had only skin=minerva before. Going that route changes other parts of the CSS that leads to undesired side-effects:

  • The font is now a serif font instead of sans-serif.
  • Infobox content is not centered anymore and has white background instead of gray.
BeforeAfter

useskin=minerva

This triggers the loading of skinSyles:
https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/skinStyles/mediawiki.skinning.content.parsoid/minerva.less

mediawiki.skinning.content.parsoid is not used inside the Minerva skin outside VisualEditor, so feel free to put any styles in that file above that you need! I suspect it is incomplete. Note the order of modules in your request is important, it might be beneficial to include mediawiki.skinning.content.parsoid module last. I suspect the font changes might be in a different module. When you inspect are you seeing any overrides? Knowing the exact CSS setting the font would be useful.

  1. Any update?
  2. What is responsible for trimming the mediawiki.skinning.content.parsoid? e.g

https://en.wikipedia.org/w/load.php?debug=false&lang=en&skin=minerva&target=mobile&only=styles&modules=mediawiki.skinning.content.parsoid
vs
https://en.wikipedia.org/w/load.php?debug=false&lang=en&skin=FAKE_STYLE&target=mobile&only=styles&modules=mediawiki.skinning.content.parsoid

On local dev setup, I'm not able to reproduce it. (though I don't have all the extensions, but I do have MobileFrontend and MobileApp)

skinstyles are specific to the skin. As described in https://phabricator.wikimedia.org/T198534#4889186 minerva screen defines its own styles and the new styles need to be added here.

skinstyles are specific to the skin. As described in https://phabricator.wikimedia.org/T198534#4889186 minerva screen defines its own styles and the new styles need to be added here.

I don't understand how to replicate this in local dev setup.
What extensions/skins are required to get it reproduced? (MobileFrontend?Minerva? MobileApp? all are installed and plain MW doesn't have this issue)
any special LocalSettings?

@eranroz the URL [1] you are using asks for the ResourceLoader module mediawiki.skinning.content.parsoid in mobile target mode for skin Minerva
Thus Minerva and core is all that's required to obtain this URL.

Documentation on how skinStyles works can be found here:
https://www.mediawiki.org/wiki/Manual:$wgResourceModules#Details
https://www.mediawiki.org/wiki/Manual:$wgResourceModuleSkinStyles
(although deprecated the information contained is relevant about how they work)

Practically, all skinStyles are defined here:
https://github.com/wikimedia/mediawiki-skins-MinervaNeue/tree/master/skinStyles/mediawiki.skinning.content.parsoid
This CSS code is used in both apps and VisualEditor.

[1] https://en.wikipedia.org/w/load.php?debug=false&lang=en&skin=minerva&target=mobile&only=styles&modules=mediawiki.skinning.content.parsoid

eranroz added a comment.EditedMar 16 2019, 7:44 AM

some people in hewiki are asking is there any update about this task or expected ETA?
It takes more than a 0.5 year to handle CSS issues. (I know there are tons of other tasks, but please find some time for it. If we aren't sure about other CSS rules - maybe just port only the border and create a separate task to investigate the rest of the CSS) .
[If you are curious - this disscussion is in hewiki and Google Translate is good enough to understand the last section]

Thanks!

Change 497340 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Images should have borders in Android app / parsoid generated content

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

@eranroz thank you for prodding this task. Unfortunately this problem is one that spans 2 teams and 2 different skillsets, so I'm guessing the slowness/lack of an ETA is not because people don't care but because it's not clear how to facilitate this change as it's not been clear who's responsibility this is. I've used my volunteer time to submit a patch since I have the CSS expertise, which I'm hoping the Android app team can test and merge to move this along (as I can't verify this fixes the problem in apps), but that depends on what other things they have on their plate. If you can help identify people who understand the problem and have sufficient understanding of testing CSS in apps to review that patch that would also be useful!

bearND removed bearND as the assignee of this task.Mar 19 2019, 3:11 AM
bearND assigned this task to Jdlrobson.Mar 19 2019, 3:15 AM

Change 497340 had a related patch set uploaded (by Mholloway; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Images should have borders in Android app / parsoid generated content

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

Change 497340 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Images should have borders in Android app / parsoid generated content

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

bearND added a comment.EditedApr 11 2019, 8:34 PM

@Jdlrobson The border doesn't cover the whole circumference of the image. In a comment for PS2 of the patch you mentioned that a MobileFrontend dependency was missing. Which was it? I suspect we might need to add this to our base CSS output.

Jdlrobson added a comment.EditedApr 23 2019, 12:39 AM

I5c1fdfc87a48764cbb53b7a751fbffc1a4e36fc5

The styles that are needed are in mediawiki.skinning.content.parsoid and skins.minerva.content.styles - if you have local copy of the latter you may need to copy/paste.

@Jdlrobson The border doesn't cover the whole circumference of the image. In a comment for PS2 of the patch you mentioned that a MobileFrontend dependency was missing. Which was it? I suspect we might need to add this to our base CSS output.

The border doesn't cover the whole image yet. An editor in hewiki have indicated that this is most visible with lists with small flags as in this page: https://he.wikipedia.org/wiki/%D7%91%D7%90%D7%A8%D7%98_MRAD#%D7%9E%D7%A9%D7%AA%D7%9E%D7%A9%D7%99%D7%9D
The border have some padding around the images, while in mobile and desktop version it looks well.

Restricted Application changed the subtype of this task from "Task" to "Bug Report". · View Herald TranscriptJun 29 2019, 10:09 PM