Make DSImages use <source> or srcset to declaratively accommodate various card dimensions
Categories
(Firefox :: New Tab Page, task, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox77 | --- | verified |
People
(Reporter: mconley, Assigned: mconley)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
The DSImage React component uses an IntersectionObserver to detect when it intersects the viewport. At that point, the bounding client rect is examined to determine what the src of the underlying image should be.
This is presenting a challenge for the about:home startup cache I'm working on as part of bug 1614351, because when creating the cache in a ChromeWorker, no IntersectionObservers fire (since there's no DOM), and nothing really has dimensions, since there's no layout.
One solution to this would be to change DSImage from dynamically computing the image src inside of the React component at runtime, and instead to have Gecko choose the right image for the card dimensions. In my local testing, it appears as if Discovery Stream was designed to be responsive, and that there are really only a few different DSCard dimensions to consider. Presuming this is true, we could use <source> inside of the <picture> element for the DSImage:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture
to choose the right src to get from https://img-getpocket.cdn.mozilla.net/.
Would that be workable, or am I missing something here, gvn?
|
||
Comment 1•3 years ago
|
||
I think that may be workable. The intention was to allow the client to determine exactly the image size it needs via DOM measurement of its container dimensions, but in actuality we are doing more an adaptive layout with a limited collection of fixed dimensions instead of a fully liquid responsive layout. We will be adding more image sizes and layouts soon, so I still think there's value to dynamic computations, but if it's a significant performance problem we can migrate to fixed sets of sizes.
FWIW there is also an optimize prop on DSImage that can be switched to false to easily see perf differences between client measured sizing and single fixed sizing.
|
Assignee | |
Comment 2•3 years ago
|
||
Okay, thanks. I suspect even if down the line we end up having a fully liquid layout for the page, it wouldn't make much sense to have the Pocket thumbnail server cache every possible thumbnail size, right? Correct me if I'm wrong here, but we'd probably want a range of thumbnails generated per story, but only like... two or three. In fact, according to my testing, we only use 3 right now:
Largest: 296x148
Medium: 218x109
Smallest 202x101
So, would it be alright if I codify that inside of the DSImage component using sizes and srcset?
If we decide to allow a larger variation for DSImage (for perhaps a larger "hero" image on about:newtab), perhaps that could receive a special property that allows the component to choose a more appropriate set of sizes and srcset. Does that sound acceptable?
|
|
||
Updated•3 years ago
|
|
|
Assignee | |
Comment 3•3 years ago
|
||
|
||
Updated•3 years ago
|
|
|
Assignee | |
Comment 4•3 years ago
|
||
This latest patch has the added bonus where if the user had a small window to start with when the page loads, if they then grow the window to be to a larger size, the thumbnails will improve quality, as they'll dynamically update to the right resolution based on the viewport.
|
||
Updated•3 years ago
|
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d4b290204251 Make DSImages use native lazy loading, and declarative responsive images. r=thecount
|
||
Comment 6•3 years ago
|
||
| bugherder | ||
|
||
Comment 7•3 years ago
|
||
I have verified, using the "Inspect Element" tool, that the "sizes="(min-width: 1122px) 296px,(min-width: 866px) 218px,(max-width: 610px) 202px,202px" and the <scrset> values were added to the DS images.
Verified using the latest Firefox Nightly (77.0a1 Build ID - 20200428214747)installed on Windows 10 x64, Mac 10.15.4, and Ubuntu 18.04 x64.
Description
•