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

[Web] fix Image fit when using ImgElementPlatformView #164400

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GiacomoPignoni
Copy link

@GiacomoPignoni GiacomoPignoni commented Feb 28, 2025

Hello, I've found the issue #163288 and I've tried to fix it.
The problem was that, to make the image appear in the right way based on the given BoxFit, it was needed also set the right object-fit on the HTML <img> element.
So I've added it to the ImgElementPlatformView widget and I've also added some convenience thing in web.dart to set the object-fit property

Fixes #163288

Here is the result

before
Screenshot 2025-02-26 230655

after
Screenshot 2025-02-26 230732

Pre-launch Checklist

@flutter-dashboard
Copy link

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.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 28, 2025
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Hi @GiacomoPignoni welcome! It looks like this change does not have tests. We'll need those to ensure this works as expected, and to prevent regressions in the future. Can you add tests to this change?

@GiacomoPignoni
Copy link
Author

Hello @Piinks
I've added the tests, I've took inspiration from selectable_region_context_menu_test.dart

@justinmc
Copy link
Contributor

@GiacomoPignoni Thanks for the tests, but it looks like there are some failed checks now, can you take a look?

@GiacomoPignoni
Copy link
Author

GiacomoPignoni commented Mar 19, 2025

@justinmc I'm sorry but I'm not really good at understating the problem
I've solved the formatting issue, but it seems there still some error about SemanticsRole, even though I didn't touch it in this PR

Versions used: 6_1_0
Tokens used: 1040/2268

To see detailed version and token usage, run with --verbose (-v).
ELAPSED TIME: 0.809s for bin/cache/dart-sdk/bin/dart --enable-asserts dev/tools/gen_defaults/bin/gen_defaults.dart in /b/s/w/ir/x/w/flutter
▌01:15:28▐ Up to date integration test template files...
▌01:15:28▐ Analysis failed.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
The error messages reported above are repeated here:
  -- This line intentionally left blank --  
Command: bin/flutter analyze --flutter-repo
Command exited with exit code 1 but expected zero exit code.
Working directory: /b/s/w/ir/x/w/flutter
stdout and stderr output:
Analyzing flutter...                                            

  error • The type 'SemanticsRole' is not exhaustively matched by the switch cases since it doesn't match 'SemanticsRole.status' • packages/flutter/lib/src/semantics/semantics.dart:106:67 • non_exhaustive_switch_expression

1 issue found. (ran in 234.1s)
  -- This line intentionally left blank --  
Dart analyzer failed when --watch was used.
Command: bin/flutter analyze --flutter-repo --watch --benchmark
Working directory: /b/s/w/ir/x/w/flutter
stdout and stderr output:
Analyzing Flutter repository...                                    19.0s


  error • The type 'SemanticsRole' is not exhaustively matched by the switch cases since it doesn't match 'SemanticsRole.status' • packages/flutter/lib/src/semantics/semantics.dart:106:67 • non_exhaustive_switch_expression
1 issue found. (1 new) • analyzed 5176 files (ran in 19.02s)
Analysis benchmark written to analysis_benchmark.json ({time: 19.017, issues: 1}).

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
You may find the errors by searching for "╡ERROR #" in the logs.
╔═╡ERROR #1╞════════════════════════════════════════════════════════════════════
║ Command: dart --enable-asserts /b/s/w/ir/x/w/flutter/dev/bots/analyze.dart
║ Command exited with exit code 1 but expected zero exit code.
║ Working directory: /b/s/w/ir/x/w/flutter
╚═══════════════════════════════════════════════════════════════════════════════
▌01:15:28▐ Test failed.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
The error messages reported above are repeated here:
  -- This line intentionally left blank --  
Command: dart --enable-asserts /b/s/w/ir/x/w/flutter/dev/bots/analyze.dart
Command exited with exit code 1 but expected zero exit code.
Working directory: /b/s/w/ir/x/w/flutter
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
You may find the errors by searching for "╡ERROR #" in the logs.

@Piinks
Copy link
Contributor

Piinks commented Mar 19, 2025

Hi @GiacomoPignoni have you tried rebasing with the tip of tree to resolve the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The image aspect ratio is distorted when using the webHtmlElementStrategy property.
3 participants