Page MenuHomePhabricator

Make black background in gallery work on device
Closed, ResolvedPublic

Description

Issue

Initially, we were using a black background in gallery but portrait images were displaying horizontally stretched (T248443). In fixing that issue, we learned that the black background is not being displayed correctly on portrait images. This issue is present only on device, the browser shows the correct behavior. After several failed attempts, we've decided to leave the background white only temporarily so as to not hold up the release for this.

What follows is a dump of everything we've learned and gathered so far on this issue.

Notes

The black background adds value to the app experience, it gives the gallery a bit of a special cinema-like feel 📽

The bug is only present when interacting with app on device, the browser works as expected. This makes it a bit trickier to debug. We can’t quite get the behavior on the browser and on the device to reconcile. We have tried a few different things:

  • setting a combination of different properties on flex child item img
    • using align-self to override the default value [1]
    • setting flex [2]
    • using flex-basis [3]
  • adjusting padding [4]

In all those case we either end up with images still stretched vertically or horizontally, or the white background on landscape or portrait. Could it be some of the flex properties not being supported or recognized on device? The issue is getting the same for both landscape or portrait: what's working one way is at the sacrifice of the other.

Note that the cat article has a good gallery to test this bug with because it contains both landscape and portrait images, as well as one svg for which we need the white background on image for (more about that in T236302). Booting up the WebIDE (for on-device debugging) we can further confirm that behavior of the img size overlapping parent divs, here with a red border to visualize debugging:

We also tried something perhaps a bit more out there [5] with an extra parent div, and this was looking okay(ish?) on device but broken on browser. This I'm leaving in sample branch T248443-images-jio2-gallery-img-idea.

Solution ideas

  • One possible solution the solution is to constrain width OR height based on the aspect ratio of the image vs. the aspect ratio of the space available
  • Is there a way to know beforehand via the respective API call if the image is portrait or landscape? With that info, we could perhaps set the flex properties dynamically
  • if for some reason we had to move away from flex in gallery thumbnail, we could simulate what's being done on desktop Wikipedia using display: table instead

Links

[1] - https://stackoverflow.com/questions/37609642/why-does-flexbox-stretch-my-image-rather-than-retaining-aspect-ratio
[2] - https://stackoverflow.com/questions/29467660/how-to-stretch-children-to-fill-cross-axis
[3] - https://stackoverflow.com/questions/34352140/what-are-the-differences-between-flex-basis-and-width
[4] - https://stackoverflow.com/questions/44009015/use-flexbox-and-maintain-a-11-aspect-ratio-even-though-content-is-sized-differe
[5] - https://stackoverflow.com/questions/30330845/flexbox-image-scale-to-height-and-keep-aspect-ratio

Event Timeline

eamedina created this task.Apr 20 2020, 2:24 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 20 2020, 2:24 AM
SBisson moved this task from Backlog to Next up on the Inuka-Team board.May 12 2020, 3:00 PM
AMuigai triaged this task as Medium priority.May 12 2020, 6:37 PM
AMuigai moved this task from Next up to Kanban on the Inuka-Team board.May 13 2020, 11:34 AM
AMuigai edited projects, added Inuka-Team (Kanban); removed Inuka-Team.
eamedina claimed this task.May 18 2020, 9:26 PM
eamedina moved this task from Ready for Dev to Dev on the Inuka-Team (Kanban) board.
eamedina moved this task from Dev to Code Review on the Inuka-Team (Kanban) board.May 27 2020, 1:32 PM

Note that you will see a blink changes to the image size, this can be (hopefully) fixed by covering it when having the loading gif layer in T253466: [IMPROVEMENT] add a loading gif on the gallery when the image is loading

hueitan moved this task from Code Review to QA on the Inuka-Team (Kanban) board.May 28 2020, 8:45 AM
Jpita moved this task from QA to Design sign off on the Inuka-Team (Kanban) board.May 29 2020, 4:38 AM

Working as expected, moving it for product signoff.

AMuigai added a subscriber: AMuigai.Jun 2 2020, 1:16 PM

Is this in production? I still see a white background in some images: examples can be seen in the articles https://en.wikipedia.org/wiki/Nairobi and https://en.wikipedia.org/wiki/Nairobi_National_Park

Looking at the Nairobi article, this is the one picture I'm seeing that I believe you are referring to:

What's happening in this case is that part of the image is transparent. Many articles have images of this type, known as SVGs, where part or all of the 'background' of the image itself is transparent. To correctly handle the SVG cases, what we're doing is that we apply a white background to the image itself (as opposed to the background of the gallery screen where the image is displayed, which is black). This is what we're doing on the iOS app as well, see this thread for more info.

This is what the 'raw' SVG above looks like:

The gray squares represent the transparent part. In order to fully appreciate the white background solution for SVGs, check out this case (cat article, fourth image in gallery):

Raw imageWith white backgroundWithout white background

Does this make sense? I wasn't able to find an image with white background in the Nairobi National Park article (tested on banana phone). And yes, this branch has been merged to master already.

AMuigai closed this task as Resolved.Jun 4 2020, 2:12 PM

Seeing some images with a white background on Jio 2, will file a new bug.