Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@flutteractionsbot
Copy link

@flutteractionsbot flutteractionsbot commented Dec 6, 2024

This pull request is created by automatic cherry pick workflow
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.

Issue Link:

What is the link to the issue this cherry-pick is addressing?

flutter/flutter#158961

Changelog Description:

Explain this cherry pick in one line that is accessible to most Flutter developers. See best practices for examples

Fix an issue on iOS 18.2 where web view's link is not tappable.

Impact Description:

What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch)

Web view's link is not tappable on iOS 18.2

Workaround:

Is there a workaround for this issue?

No workaround

Risk:

What is the risk level of this cherry-pick?

  • Low
  • Medium
  • High

Test Coverage:

Are you confident that your fix is well-tested by automated tests?

  • Yes
  • No

Validation Steps:

What are the steps to validate that this fix works?

  1. Launch web view plugin's example app
  2. Tap on ... menu on top left corner
  3. Tap on any link on web view. The link is working with the fix, and not working without the fix.

This is pretty tricky bug - I suspect that because Apple's internal recognizer caching an outdated state of our delaying recognizer. 

The conflict happens between WebKit and platform view's recognizers. It happens to all plugins that uses a WKWebView, or any view that has a similar (unknown) gesture setup. 

This fix has to be in the engine (rather than the plugin), because the plugin itself knows nothing about the existence of our delaying recognizer. 

Here are the steps of my research for future reference: 

1. The bug only happens when the overlay flutter widgets blocks the gestures for the platform view (e.g. tap on the platform view area when a flutter context menu is displayed). When the bug happens, WKWebView's link doesn't work anymore, however, the link can still be highlighted when tapped. 

2. When I remove the delaying recognizer from platform view, the link became clickable again. This means that the bug is related to some conflict between WKWebView's internal recognizers and our delaying recognizer. This also means that it is not possible to fix this issue at plugin level, which knows nothing about the delaying recognizer. 

3. When we tap on the web view when context menu is displayed, `blockGesture` will be called, which simply toggles delaying recognizer's state from `possible` to `ended` state, meaning it should block all recognizers from the current gesture (and it correctly did so). Then I use `dispatch_async` and check the state again, and confirmed the state is correctly reset to `possible` state. 

4. Subsequent tap on web view triggers `acceptGesture`, which turns the `possible` state into `failed` state. This subsequent tap only highlights the link, but not activate the link. This suggests that some internal web kit recognizer that handles the highlight sees the `failed` state of delaying recognizer (which is correct), but the recognizer that handles the link activation probably sees the stale state of `possible` or `ended` (which is outdated old state). 

5. So the solution is trying to make the recognizer "see" the updated state rather than the cached old state. 

6. I tried recreating a new delaying recognizer when `blockGesture` is called: 
```
- blockGesture {
  delayingRecognizer.state = .ended
  dispatch_async {
    // force re-create the delaying recognizer
  }
}
```
This fixed the link activation bug, however, when opening the context menu again, the gesture is not blocked anymore. This means web kit internal recognizers likely cache the old delaying recognizer for state update, thus the new instance of delaying recognizer won't work anymore. So we can't change the instance. However, it's a good experiment that confirms my hypothesis that some internal webkit recognizer caches the outdated state of delaying recognizer. 

7. For the above code, rather than using `dispatch_async`, I also tried `dispatch_after`, and it turns out that it only works if the dispatch_after delay is `0` - even if the delay is much smaller than 1 frame's time (16.7ms), it doesn't work. This means the state checking happens either at the end of the current run loop, or beginning of the next run loop. (not too important information, but it helps me better understand how UIKit works). 

8. So from 6, we know that we have to keep the original instance of delaying recognizer. I tried toggling `recognizer.enabled`, it didn't work. I also tried inserting a dummy recognizer, it didn't work. Neither approach triggers the state "refresh" for those webkit internal recognizers. 

9. I tried removing and adding back the delaying recognizer, and it just worked! This means that removing and adding back the delaying recognizer probably triggered UIKit to refresh the states for all its related recognizers (i.e. those recognizers either blocking or being blocked by the delaying recognizer), hence getting the updated state. 

*List which issues are fixed by this PR. You must list at least one issue.*

Fixes flutter/flutter#158961

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@flutteractionsbot flutteractionsbot added the cp: review add the cp request to the review queue of release engineers label Dec 6, 2024
@flutteractionsbot
Copy link
Author

@hellohuanlin please fill out the PR description above, afterwards the release team will review this request.

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@christopherfujino christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 6, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 6, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Dec 6, 2024

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

@christopherfujino
Copy link
Contributor

Getting compiler errors (I'm guessing because certain symbols don't exists on this branch?):

 ../../../flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm:2834:3: error: unknown type name 'FlutterPlatformViewsController'; did you mean 'FlutterPlatformViewIdentifier'?
  2834 |   FlutterPlatformViewsController* flutterPlatformViewsController =
       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       |   FlutterPlatformViewIdentifier

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8729259887208561569/+/u/build_ci_ios_debug_sim/stdout

@christopherfujino
Copy link
Contributor

Closing in favor of #57034

@hellohuanlin
Copy link
Contributor

Side note: we should probably refrain from doing large refactoring work such as moving files or renaming right before the cut.

Copy link

@saltman007web saltman007web left a comment

Choose a reason for hiding this comment

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

someone link me install

auto-submit bot pushed a commit that referenced this pull request Dec 9, 2024
…view #57030 (#57034)

Manual version of #57030 (comment)

Since I can't push it to the original branch. 

This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request)
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.

### Issue Link:
What is the link to the issue this cherry-pick is addressing?

flutter/flutter#158961

### Changelog Description:
Explain this cherry pick in one line that is accessible to most Flutter developers. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples

Fix an issue on iOS 18.2 where web view's link is not tappable. 
 
### Impact Description:
What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch)

Web view's link is not tappable on iOS 18.2

### Workaround:
Is there a workaround for this issue?

No workaround

### Risk:
What is the risk level of this cherry-pick?

### Test Coverage:
Are you confident that your fix is well-tested by automated tests?

### Validation Steps:
What are the steps to validate that this fix works?

1. Launch web view plugin's example app
2. Tap on `...` menu on top left corner
3. Tap on any link on web view. The link is working with the fix, and not working without the fix.
phamnhuvu-dev pushed a commit to phamnhuvu-dev/engine that referenced this pull request Jan 7, 2025
…view flutter#57030 (flutter#57034)

Manual version of flutter#57030 (comment)

Since I can't push it to the original branch.

This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request)
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.

What is the link to the issue this cherry-pick is addressing?

flutter/flutter#158961

Explain this cherry pick in one line that is accessible to most Flutter developers. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples

Fix an issue on iOS 18.2 where web view's link is not tappable.

What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch)

Web view's link is not tappable on iOS 18.2

Is there a workaround for this issue?

No workaround

What is the risk level of this cherry-pick?

Are you confident that your fix is well-tested by automated tests?

What are the steps to validate that this fix works?

1. Launch web view plugin's example app
2. Tap on `...` menu on top left corner
3. Tap on any link on web view. The link is working with the fix, and not working without the fix.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cp: review add the cp request to the review queue of release engineers platform-ios

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants