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

#10 fix #49

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

#10 fix #49

wants to merge 7 commits into from

Conversation

wolfenrain
Copy link

@wolfenrain wolfenrain commented Mar 1, 2018

Fix for some changes in #10

General:

Changed the way how scrolling of native elements work. We add a scroll EventListener to the document. And then on scroll we check if the scrolling target contains video-elements with the data-stream-id property. If so then we send the position(relative to the viewport, changed in the getPosition) of the video-element with the stream-id to the native side to update its position.

By doing this JavaScript implementation, we are now capable of scrolling multiple native video-views, independently within their own HTML parent. Which wasn't possible before using native scrollviews.


There are also some changed that are platform specific.

Android:

  • Removed the observable scroll view. It caused the webview to become as high as its content, instead of as high as the viewport.

iOS:

  • Added the cordova-plugin-wkwebview-engine as its main WebViewEngine, because the default from Cordova is the UIWebView, and it stops javascript-execution while you are scrolling. This dependency is only needed until Cordova implements the wkwebview self.
  • We also added a check if the cordova-plugin-statusbar is loaded. Because that plugin changes the origin position of the webview. Normally the top position is 0 and its below the statusbar. But that means that the native views should have 20 pixels more. Otherwise they will be on top of the statusbar. But when the StatusBarPlugin is loaded, the top position of the webView is on top of the statusbar(still 0). So a small ifstatement is made to check if we should add a small offset or not.

Those are in general the major/important changes.

@mark-veenstra
Copy link

@msach22 This one is in WIP

@msach22 msach22 changed the title #10 fix [WIP]#10 fix Mar 7, 2018
@wolfenrain wolfenrain changed the title [WIP]#10 fix #10 fix Mar 8, 2018
@wolfenrain
Copy link
Author

@msach22 Removed WIP, should all be fixed now.

@msach22
Copy link

msach22 commented Mar 15, 2018

@wolfenrain @mark-veenstra have you noticed any performance issues or lagging with this scrolling solution? AFAIK, the scroll event is only available after the scroll has completed, is this the same in this case?

@mark-veenstra
Copy link

mark-veenstra commented Mar 15, 2018

@msach22 only on iOS we experienced the issue that the scroll event was available after releasing the scroll. But this is solved in the new WKWebView of iOS. Therefore we added the dependency on WKWebView, which is also the future WebView for Cordova instead of the UIWebView. So this dependency will be obsolute over time.

On Android it worked without any other addition.

@wolfenrain wolfenrain changed the title #10 fix [WIP] #10 fix Mar 15, 2018
@wolfenrain
Copy link
Author

@msach22 that is correct. This PR contains the new WKWebView, because the original UIWebView is blocking javascript when scrolling. So you only get the scroll event after you are done scrolling. That has been fixed in the new WKWebView

Also put the PR back to WIP because there are a few more things I need to be sure about before we merge.

@wolfenrain wolfenrain changed the title [WIP] #10 fix #10 fix Mar 19, 2018
…ntok into issue-10-fix

# Conflicts:
#	build-extras.gradle
@wolfenrain
Copy link
Author

@msach22 I removed the WIP, ready for merge.

@wolfenrain wolfenrain changed the base branch from 3.2.0 to master May 1, 2018 08:48
@mark-veenstra
Copy link

@msach22 Can we get this one out? Maybe in a 3.4.0?

@msach22
Copy link

msach22 commented Jun 8, 2018

@wolfenrain @mark-veenstra Yes, let's aim to get to this out in the next feature release. I'll review this PR in the upcoming week.

@wolfenrain
Copy link
Author

wolfenrain commented Aug 9, 2018

@msach22 @mark-veenstra According to this blog post by Cordova, the WKWebview is gonna become the default in new iOS platform version(5). Considering that its related to this issue, because its added as a dependency, I thought it would be useful to reference it here.

@mark-veenstra
Copy link

Also asked a question about this plugin and the new bridge within the [email protected] here.

@mheap mheap changed the base branch from master to main March 22, 2021 09:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants