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

[iOS] startInLoadingState with useWebKit breaks initial position on website #474

Closed
winkelsdorf opened this issue Apr 3, 2019 · 12 comments

Comments

@winkelsdorf
Copy link

Using startinLoadingState used together with useWebKit currently breaks the initial position of a loaded website on iOS:

Expected and actual screenshots both reflect the initial state after loading.

Expected
Screen Shot 2019-04-03 at 14 14 55

Actual
Screen Shot 2019-04-03 at 14 14 51

Steps to reproduce

  • Use WebView with startInLoadingState and useWebKit both set to true
  • Set any website as source
  • Notice website is initially scrolled down a bit when loaded

Simple demo to reproduce

/**
 * @format
 * @flow
 */

import React, { Component } from 'react';
import { View } from 'react-native';
import { WebView } from 'react-native-webview';

type Props = {}

export default class App extends Component<Props> {
    render() {
        return (
            <WebView
                source={{uri: 'https://facebook.github.io/react-native/'}}
                startInLoadingState={true}
                useWebKit={true}
            />
        );
    }
}

Turning off either makes it work like expected.

Environmnent
react-native@0.59.3
react-native-webview@5.6.0

Verified bug on

  • iOS Simulator iPhone X - iOS 12.2
  • iOS Simulator iPhone 4s - iOS 9.0
  • Device iPhone X - iOS 12.2
@winkelsdorf winkelsdorf changed the title startInLoadingState with WKWebView breaks initial position on website startInLoadingState with useWebKit breaks initial position on website Apr 3, 2019
@winkelsdorf
Copy link
Author

winkelsdorf commented Apr 3, 2019

Probably related to http://www.openradar.me/22855188.

Putting a breakpoint in RNCWKWebView.m shows the frame to be zero. Loading content while offscreen might trigger the bug. I'm just wondering why this does not happen when startInLoadingState is false, as there is no relation at all.

In RNCWKWebView.m:

- (void)didMoveToWindow
...
    CGRect test = self.bounds; // <- added for easier debugging
    _webView = [[WKWebView alloc] initWithFrame:self.bounds configuration: wkWebViewConfig];
    _webView.scrollView.delegate = self;
    _webView.UIDelegate = self;
    _webView.navigationDelegate = self;
    _webView.scrollView.scrollEnabled = _scrollEnabled;
    _webView.scrollView.pagingEnabled = _pagingEnabled;

Screen Shot 2019-04-03 at 14 49 43

Edit: Nevermind. The initial frame and bounds are always zero, as we are offscreen. The error seems to be related to the handling of _shouldStartLoadLock in RNCWKWebViewManager.m.

Probably some kind of layout race condition..

@winkelsdorf
Copy link
Author

winkelsdorf commented Apr 3, 2019

Works when commenting out the line with UIScrollViewContentInsetAdjustmentNever (L151 in RNCWKWebView.m).

That was or is probably needed as a workaround for something else like mentioned in the radar report from above comments.

But

In https://stackoverflow.com/a/46466883/844907 the Apple Engineer smileyborg mentiones regarding the UIScrollViewContentInsetAdjustmentNever hotfix:

This issue was caused by a bug in iOS 11 where the safeAreaInsets of the view controller's view were set incorrectly during the navigation transition, which should be fixed in iOS 11.2. Setting the contentInsetAdjustmentBehavior to .never isn't a great workaround because it will likely have other undesirable side effects. If you do use a workaround you should make sure to remove it for iOS versions >= 11.2

-mentioned by smileyborg (Software Engineer at Apple)

Emphasis mine.

Probably the define check needs adjustment. __IPHONE_OS_VERSION_MAX_ALLOWED just checks for the SDK compiled against. That line would always be included for SDK 11+, while it should probably be disabled for anything after SDK 11.2.

Note: I am not aware of a check that is able to do this as a compiler macro.

Edit: Also an issue in upstream RN Core in RCTSafeAreaView.m and RCTScrollView.m.

@winkelsdorf winkelsdorf changed the title startInLoadingState with useWebKit breaks initial position on website [iOS] startInLoadingState with useWebKit breaks initial position on website Apr 4, 2019
@winkelsdorf
Copy link
Author

Addendum: Same error when using webView.reload(). Seem it is related to the implementation of the loading activity view.

@Titozzz
Copy link
Collaborator

Titozzz commented Apr 20, 2019

@winkelsdorf feel free to submit a PR

@winkelsdorf
Copy link
Author

@Titozzz Would love to, but I'm an iOS (Swift) Developer and have no clue where to start with such bugs in RN components.

@Titozzz
Copy link
Collaborator

Titozzz commented Apr 20, 2019

Well the part broken is in the iOS code it seems so you should be able to update it :)

@winkelsdorf
Copy link
Author

@Titozzz Sorry, looks like it's not that easy. That needs some help of a component aware RN/iOS developer. That's why I stopped after several hours of debugging and reported this issue.

E.g. the handling of _shouldStartLoadLock and the unusual construction of WKWebView in didMoveToWindow need explanation. Whats the reason for those, as in the wrapped UIWebView the usual constructor is used.

There has to be an undocumented reason that somebody did this? If I start changing things without knowing about the, I would likely break other things.

If anyone is willing to explain, join discussion or contribute, let me know.

@winkelsdorf
Copy link
Author

winkelsdorf commented Apr 23, 2019

As said it's not that easy. Spent 4 hours already, current observations:

  • The initial frame of the superView and _webView is 0,0,0,0 (origin x, y, width, height)
  • A size of 0,0,375,0 follows - so we have a non-null rect with zero height
  • Finally the size is correct with 0,0,375,641 (size for an iPhone X with TabBar)

As the frame of the _webView is always set to superView size in layoutSubviews several things are misaligned:

  • Initial scroll position when using startInLoadingState
  • Reload makes scroll position jump down a bit with each reload (till end of page is reached)

Ignoring zero rects with CGRectIsNull or empty ones with CGRectIsEmpty does not solve all problems (reload works when checking for CGRectIsEmpty but breaks initial position, too).

I am currently missing the link between the loading view on RN side and the frame bounds in the objc side.

Edit: I guess setting height: 0 during load in WebView.styles.js hidden is related to this..
Edit 2: As expected commenting webViewStyles.push(styles.hidden); in L275 of WebView.ios.js makes WKWebView work as expected.

RN code changes height to 0 several times (hidden during activity, on error) which of course breaks native scrollView handling.

@Titozzz So, what should I do now? Looks like the broken part is in the RN code. How to you hide a view without setting height to 0?

Edit 3: Setting style to display: none; has the same issues.

To fix also Androids ErrorView (https://stackoverflow.com/questions/55489803/remove-android-default-error-page-on-react-native-webview/) I would suggest changing the whole logic to something like an overlay on the RN side.

@Titozzz
Copy link
Collaborator

Titozzz commented Apr 24, 2019

I've thought about this many times. I could try to do that. I'll make a branch you can test

@winkelsdorf
Copy link
Author

@Titozzz Great idea!

As this would also fix the issues with the Android Error Page, feel free to close this issue and put it on a roadmap. On the other hand I'm going to sort some smaller issues out with the native implementation (e.g. unneeded forced enum casting) and try to understand the old logic and do a bit of a rewrite for the iOS Part.

As RN support iOS 9+ only, some of the keyboard handler fixes might be removed as well.

Thanks for your help 👍

@Titozzz
Copy link
Collaborator

Titozzz commented Jun 3, 2019

should be fixed since 5.11.0

@Titozzz Titozzz closed this as completed Jun 3, 2019
@winkelsdorf
Copy link
Author

@Titozzz Thanks for the update!

Initial position works as expected now.
The overlay is working, too - but the Android Error page still flashes shortly.

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

No branches or pull requests

2 participants