[Web] Allow specifying the strategy on when to use <img> element to display images

[dkwingsmt]

This PR follows the discussion under #157755 and adds a flag to determine when <img> elements are used. By default the feature is turned off.

Instead of just a boolean for on & off, I made it an enum to accept multiple options. Notably, an always option can be useful when the developer wants a unified experience regardless of the image origin (such as when displaying an image from arbitrary URLs.)

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• I followed the breaking change policy and added Data Driven Fixes where supported.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[slavap]

@dkwingsmt thank you for working on this, it is definitely the right thing to add.

[flutter-dashboard]

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[jezell]

This is great. Making this behavior opt in will save a lot of frustration. Nice callout in the docs about the potential pitfalls and limitations.

[Levi-Lesches]

Given the "drawbacks" section, it would be nice to log something here to notify the dev that the workaround was needed. That way the dev can be more mindful of CORS errors, and avoid doing double the work in the future by changing the code to .always

[dkwingsmt]

I kind of get the idea but not completely... Can you provide a suggestion?

[Levi-Lesches]

Probably using some special Flutter logging function, not print, but something like this:

} catch (error) {
  if (WebImgElementStrategy.warnOnCors) {
    print("Warning: Could not load the network image at the following URL: $url");
    print("  The error was: $error");
    print(""); 
    print("  This could indicate that the CORS on the image or server is not configured properly.");
    print("  To load your image, Flutter used a native HTML <img> element to render the image without CORS.");
    print("  However, this is not without its drawbacks. For more details, see <url to Image.network docs>");
    print("");
    print("  Consider instead: ");
    print("    - Configuring CORS correctly on your server, and pass WebImgElementStrategy.never. See <url to web.dev>");
    print("    - Explicitly setting WebImgElementStrategy.always to save Flutter time deciding which strategy to use");
    print("");
    print("  To suppress this warning in the future, set WebImgElementStrategy.warnOnCors = false");
  }
  return loadViaImgElement();
}

With this log statement, I'd recommend:

• setting the default strategy to WebImgElementStrategy.whenNecessary
• setting the default WebImgElementStrategy.warnOnCors = true

@jezell @slavap I know you both want to avoid slow or buggy behavior by default, but I'm still really concerned that plenty of devs, especially those that come from the mobile world, won't understand what's happening and might not know to use this option if there is no explanation. I think this gives a nice middle ground where:

• the default behavior is images mostly "just work"
• developers are warned about potentially slow or buggy behavior
• we give a link to explain CORS (eg, this one from web.dev)
• we give a link explaining the drawbacks (the Image.network docs)
• developers are first advised to fix CORS properly
• developers are given a fallback option to use the workaround and disable the warning

Would you agree this is a good compromise?

[jezell]

@Levi-Lesches No, I don't think it's not a good compromise to turn it on by default because platform views themselves don't just work. Platform views are a hack with too many downsides to enable by default. I've spent hundreds of hours dealing with fallout from using them. They are by far the most error prone part of Flutter web. We'd be far better off if time if the entire HtmlElementView widget was deleted and people were forced to use the web package to insert html elements in the page when that's what they want (which they can already do today).

Unfortunately, CORS is a part of the web. People need to learn to deal with it, or they need to not ship web apps.

[jezell]

@Levi-Lesches while I understand while you are coming from, I think its very important that usability never comes at the expense of stability or reliability. Any feature that breaks other features, or potentially puts the stability of your app at risk shouldn't be the default path.

I think maybe something that would be a middle ground would be to do something like those overflow graphics that appear in dev mode, and maybe render yellow text over a background color that explains that the image was blocked by the CORS policy and writes a warning to the console with a link to the docs about this feature. Providing additional guidance at dev time can be very helpful for new developers.

[Levi-Lesches]

So instead of defaulting to whenNecessary, still default to .never, but print a warning like the above when CORS fails so that devs have an actionable next step. If that's what you're saying, I agree

[dkwingsmt]

This is a nice idea, but I think I'd better make it a separate PR so that we can focus on changing the default behavior in this PR. Thanks for the suggestion!

[yjbanov]

Consider documenting what happens if an image fails to load through a fetch.

[yjbanov]

Since it's not guaranteed that an img tag will be used, I'd call this one prefer (or preferPlatformView for more specificity). If we call this prefer, then whenNecessary could be called avoid. Essentially the full scale of options could be: never (fails on cross-origin images), avoid, prefer, always (fails if headers are not empty).

[mdebbar]

I agree with prefer or something similar. But I'm not sure about avoid. It seems a little ambiguous and could be interpreted as never. I prefer fallback.

Another option is whenNecessary (only use img when necessary) and whenPossible (use img whenever possible). But I don't like the extra verbosity.

[mdebbar]

Regardless of the name, I think we should print a warning when headers is present but we can't fetch image bytes. Let's make it easier for users to know why their images are not showing up.

[yjbanov]

Agreed, fallback sounds better.

[yjbanov]

img is a bit of a implementation detail that could theoretically change. For example, we could switch to <picture>, SVG, or tainted 2D canvas for rendering CORS images. I'd therefore avoid putting "Img" in the name of the enum.

How about we call it just SameOriginStrategy?

[yjbanov]

useImageElement sounds like a boolean property. I'd call it *Strategy, e.g. sameOriginStrategy (especially if we rename the enum to SameOriginStrategy.

[mdebbar]

I agree with prefer or something similar. But I'm not sure about avoid. It seems a little ambiguous and could be interpreted as never. I prefer fallback.

Another option is whenNecessary (only use img when necessary) and whenPossible (use img whenever possible). But I don't like the extra verbosity.

[mdebbar]

Regardless of the name, I think we should print a warning when headers is present but we can't fetch image bytes. Let's make it easier for users to know why their images are not showing up.

[dkwingsmt]

I've addressed all the other comments, and what remains is naming. I'm summarizing the current options as follows:

If we use SameOriginStrategy sameOriginStrategy, then the enum name is not immediately obvious what the strategy is about, therefore the option name should be more informative, and I'll go for something like:

enum SameOriginStrategy {
  neverUsePlatformView,
  fallbackToPlatformView,
  preferPlatformView,
}

If we want to have simpler names, then I think the "platform view" part should be part of the type name, which is something like:

enum WebPlatformViewStrategy {
  never,
  fallback,
  prefer,
}

(I prefer "never" over "avoid" because "avoid" sounds like there are unavoidable cases that still use platform views.)

What are your opinions on the options? @yjbanov @mdebbar

[mdebbar]

@dkwingsmt I like both suggestions and I don't have a preference.

But given that platform views are the main concern leading to this PR, I would say let's go with the name that mentions platform views?

[slavap]

@dkwingsmt @mdebbar
IMO having enum with PlatformView wording is wrong idea, because CORS implementation may change in the future from platform views to something different, e.g.

See if we can use Canvas 2D as a target surface (instead of WebGL or bitmaprenderer). This would allow us to draw cross-origin images into the canvas directly without having to split it. We'd probably need to introduce a new kind of platform view (let's call it "drawable platform view"). Such platform views could be drawn into the canvas, but they would (necessarily) taint it (which is nothing new; that's what happens in normal HTML, including Flutter Web's HTML renderer).

see #157755 (comment)

[dkwingsmt]

That's a good point. What do you think if we use "HTML Element" as the name? No matter whether we use platform views or not, or whatever tags, we're using an HTML element to work around the CORS restriction (if I understand correctly).

enum SameOriginStrategy {
  neverUseHtmlElements,
  fallbackToHtmlElements,
  preferHtmlElements,
}

enum WebHtmlElementStrategy {
  never,
  fallback,
  prefer,
}

[slavap]

@dkwingsmt definitely better.

[auto-submit]

auto label is removed for flutter/flutter/159917, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.