Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[webview_flutter] Add support for js alert, confirm, prompt. #2012

Closed

Conversation

jerryzhoujw
Copy link
Contributor

@jerryzhoujw jerryzhoujw commented Aug 25, 2019

Description

Add support for javascript alert, confirm, prompt.

jsAlertAndOther

Related Issues

Replace this paragraph with a list of issues related to this PR from the issue database. Indicate, which of these issues are resolved or fixed by this PR. Note that you'll have to prefix the issue numbers with flutter/flutter#.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@jerryzhoujw
Copy link
Contributor Author

fix 30358

Copy link

@dieudv dieudv left a comment

Choose a reason for hiding this comment

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

I see you're only fix for ios. Can you fix for android?

@jerryzhoujw
Copy link
Contributor Author

@dieudv I'm not familiar with android develop, but I can try to take a look into it later. If anyone who familiar with android would like to fix this would be better.

@amirh
Copy link
Contributor

amirh commented Oct 10, 2019

Thank you for the contribution!

I'm following the initial PR review policy, this PR isn't trivial to review, and the Android counterpart is still massing so I'm labeling it with "backlog" and we will prioritize according to the issue's priority.

@shinriyo
Copy link
Contributor

Is it too difficult to fix for Google developers?
This issue is neglected more than one years since 25 Aug 2019.
Or, is it policy not using alert, confirm and prompt?

@stuartmorgan stuartmorgan added the p: webview_flutter Edits files for a webview_flutter plugin label Jan 29, 2021
@stuartmorgan
Copy link
Contributor

@blasten Is there a policy for changes that only affect Virtual Display mode at this point? I.e., would it be worthwhile for someone to invest time in adding tests for this if (going by the recent comment on the issue) it's not an issue with Hybrid Composition?

@stuartmorgan
Copy link
Contributor

Now that we've switched Android to hybrid composition by default, I don't think there's any reason this couldn't move forward with an iOS-only implementation.

This will need XCUITests added to cover the new functionality; please let us know when those are in place and we can review!

@godofredoc godofredoc changed the base branch from master to main January 6, 2022 22:11
@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 Hixie on the #hackers channel in Chat.

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.

@Hixie
Copy link
Contributor

Hixie commented Jan 19, 2022

@jerryzhoujw Are you still interested in working on this PR?

@jerryzhoujw
Copy link
Contributor Author

@Hixie, I'm not familiar with android develop. but ok for iOS part.

@stuartmorgan
Copy link
Contributor

I'm not familiar with android develop

As I mentioned above, now that hybrid composition is the default on Android it's fine for this PR to be iOS-only. It just needs to be updated and tested.

@jerryzhoujw
Copy link
Contributor Author

I'll do it soon.

@Hixie Hixie removed the request for review from amirh March 2, 2022 00:09
@stuartmorgan
Copy link
Contributor

Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes needs tests p: webview_flutter Edits files for a webview_flutter plugin platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants