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

[webview_flutter_web] Add the credentialless iframe attribute. #7097

Closed
wants to merge 1 commit into from

Conversation

uldall
Copy link

@uldall uldall commented Jul 10, 2024

This PR makes it possible to specify whether the <iframe> should have the attribute credentialless set to true or false.

Pre-launch Checklist

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

@uldall uldall requested a review from ditman as a code owner July 10, 2024 19:23
@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 to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

@uldall
Copy link
Author

uldall commented Jul 10, 2024

Example usage:

late final PlatformWebViewControllerCreationParams params;
if (WebViewPlatform.instance is WebWebViewPlatform) {
  params = WebWebViewControllerCreationParams(
    iFrameCredentialless: true,
  );
} else {
  params = const PlatformWebViewControllerCreationParams();
}

_webViewController = WebViewController.fromPlatformCreationParams(params);

@uldall
Copy link
Author

uldall commented Jul 10, 2024

Example of how it looks when the attribute is added:
image

@stuartmorgan
Copy link
Contributor

Thanks for the contribution! You’ve checked boxes in the PR checklist above that are not reflected in this PR, so I’m assuming this is a work in progress and am marking it as a Draft. Please review the checklist, updating the PR as appropriate, and when the checklist reflects the state of the PR as posted please feel free to mark it as ready for review.

@stuartmorgan stuartmorgan marked this pull request as draft July 10, 2024 21:40
@stuartmorgan stuartmorgan removed the request for review from ditman July 10, 2024 21:40
@uldall
Copy link
Author

uldall commented Jul 11, 2024

Thanks for the contribution! You’ve checked boxes in the PR checklist above that are not reflected in this PR, so I’m assuming this is a work in progress and am marking it as a Draft. Please review the checklist, updating the PR as appropriate, and when the checklist reflects the state of the PR as posted please feel free to mark it as ready for review.

I have added tests and comments now. I am not sure about whether I am supposed to update the pubspec.yaml and CHANGELOG.md? Sorry about the potentially stupid question. This is my first PR on the Flutter project.

@uldall
Copy link
Author

uldall commented Jul 20, 2024

Would it be preferable if I changed this PR to use a Map<String, String> of attributes instead so it became more generic?

@stuartmorgan
Copy link
Contributor

From triage: Ping @ditman on the design question above.

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Small nit with setting the credentialless attribute only when really needed.

Comment on lines 31 to 32
..setAttribute(
'credentialless', iFrameCredentialless ? 'true' : 'false'),
Copy link
Member

@ditman ditman Sep 10, 2024

Choose a reason for hiding this comment

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

Setting credentialless to any value results it in being true

Set the attribute only iFrameCredentialless is true.

@ditman
Copy link
Member

ditman commented Sep 10, 2024

Would it be preferable if I changed this PR to use a Map<String, String> of attributes instead so it became more generic?

@uldall I think it's better that we do this using the type safety of Dart and package:web rather than a stringly typed map :)

(PS: credentialless is only supported in Chrome browsers for now, we may need to add a note to the parameter itself so users are aware of this limitation?)

@ditman ditman changed the title [webview_flutter_web] Add support for the credentialless attribute. (… [webview_flutter_web] Add support the credentialless iframe attribute. Sep 10, 2024
@ditman ditman changed the title [webview_flutter_web] Add support the credentialless iframe attribute. [webview_flutter_web] Add the credentialless iframe attribute. Sep 10, 2024
@stuartmorgan
Copy link
Contributor

@uldall Are you still planning on updating this PR based on the feedback above?

@uldall
Copy link
Author

uldall commented Oct 13, 2024

I am not sure why GitHub closed my PR just because I updated my fork..

@uldall uldall reopened this Oct 13, 2024
@uldall uldall marked this pull request as draft October 13, 2024 12:42
@stuartmorgan
Copy link
Contributor

Since this is marked as a draft and hasn't been updated in several months, I’m going to close it so that our PR queue reflects active PRs. Please don't hesitate to submit a new PR if you decide to revisit this. Thanks!

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

Successfully merging this pull request may close these issues.

3 participants