Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image updates: remove BG-style, add crossOrigin functionality, allow nullable alt text #2207

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

comp615
Copy link
Contributor

@comp615 comp615 commented Jan 27, 2022

Problem
As noted in #1786 there are a few ways that images could serve customers better. In particular, this diff explicitly aims to fix: #1636 and #2171. (Happy to break this up to focus on each individually but was working on an integration test w/ Twitter)

Solution
While a full rewrite may be necessary to solve all those problems, I think a number of these issues stem from the usage of a pseudo-img-element on top of a background-image design. So if we can remove that, it may help bring focus to the remaining issues. In short, we can provide a sort of interim solution.

The crossOrigin image functionality represents a class of general problem: we cannot use all the native image functionality because we are constrained by ultimately using background image (e.g. see #1542). With privacy sandbox and site isolation coming, CORS authenticated images are necessary for us, this works OK for a normal image, but background-images have no equivalent so it's impossible to accomplish.

Getting rid of the background image also simplifies the DOM and likely fixes things like Windows High Contrast and the ML accessibility issues as well.

CORS / Cross Origin
While the RN API is to use body and headers on the source object, I do not believe this model works with the web. Because we are subject to CORS, and have server-only cookies. We necessarily need to use flags instead of explicit headers.

To solve this, I decided to add a crossOrigin mode flag matching the img element to the source definition.

This change necessitated tweaking ImageLoader for the same reason. This widens the API to take a source object not just a URI on the load method. I believe RN API also misses the mark here. If an image can be loaded with headers...how are you supposed to prefetch and object with headers? I think we should generally assume that all ImageLoader methods should take any valid source type instead of just a string URI.

Object Fit
My interpretation of the existing code is that visually, we could only get the proper resizing behavior by using background-size. This meant that for accessibility and usability (i.e. saving an image), we previously had to put an invisible "Real" img tag over the background-image. Based on that, it seems like using object-fit gives us the best of both worlds and helps simplify the code (we no longer need onLayout callbacks and so on).

The tradeoff is that browser support does not match RNW's current stated support. In particular old Edge and IE. We could get around this by using an Object fit Polyfill. So that's a disucssion.

BREAKING!: Additionally, because it's not replicable via objectFit...this drops support for the repeat resizeMode.
BREAKING!: Removed Array as a type for Source because RNW doesn't actually handle this well so feels like it should raise an issue.

Tests
I tried to verify this works in our core image cases on Twitter, but we do not exercise the full functionality of this (i.e. ImageBackground or certain image props like blur or tint).

It appeared that the image related examples looked the same before and after, and that the snapshot changes were reasonable.

Comment on lines +176 to +177
// NOTE: in order to prevent costly reloads when objects aren't equal, we only check the resolved URI here
// however, what we really want is to deepEqual test the source var on all props that might affect loading.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got the same problem for my PR: #2442 where it's possible to have source.headers

The way I handle it is to extract source to local state, and only change it when the input source have deeply changed

// Trigger a resolved source change when necessary
React.useEffect(() => {
const nextSource = resolveSource(source);
setResolvedSource((prevSource) => {
// Prevent triggering a state change if the next is virtually the same as the last loaded source
if (JSON.stringify(nextSource) === JSON.stringify(prevSource)) {
return prevSource;
}
return nextSource;
});
}, [source]);

This will in turn trigger the loading effect hook

To make this work I've changed the resolveAssetUri to resolveSource and always work with the object shape of the source. This is to make the rest of the logic simpler and not having to guess whether the source is number or anything else
But this results in some conflicts with your PR
Can you take a look and give you opinion on how to move forward - maybe we can move the hook / change detection logic to your PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my phone while traveling so not diving into the code too much. But in general, I think it would be great to patch on top of this. I strongly believe a broader image rewrite is merited, and this solves several tasks on that front.

The only issue is that I'm not sure if/when this will end up in master or the other image work will land, so it may take a bit of finagling and ordering to get all the image related patches and work together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Image to not set alt property so browsers can provide enhanced functionality
3 participants