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.

Sign up for GitHub

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

Jump to bottom

fix(android): fix onlink callback being cretion only #11177

Merged
merged 3 commits into from
Oct 2, 2019
Merged

fix(android): fix onlink callback being cretion only #11177

merged 3 commits into from
Oct 2, 2019

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Aug 26, 2019 •
edited by sgtcoolguy
Loading

(cherry picked from commit 1f51e13)

JIRA: https://jira.appcelerator.org/browse/TIMOB-27238

Description:
Fix onlink callback property being creation-only.

Test case:

var htmlText =
'<!DOCTYPE html>\n' +
'<html>\n' +
'	<head>\n' +
'		<meta name="viewport" content="width=device-width, initial-scale=1.0">\n' +
'	</head>\n' +
'	<body>\n' +
'		<p>WebView "onlink" Test</p>\n' +
'		<br/>\n' +
'		<br/>\n' +
'		<a href="mylink://show/alert">Show Alert</a>\n' +
'	</body>\n' +
'</html>\n';
 
function onLinkHandler(e) {
if (e.url === "mylink://show/alert") {
alert("'onlink' callback invoked.");
return false;
}
return true;
}
 
var window = Ti.UI.createWindow();
var webView = Ti.UI.createWebView({
html: htmlText,
//	onlink: onLinkHandler,  // <- Must be set upon creation to work.
});
webView.onlink = onLinkHandler;
window.add(webView);
window.open();

@ypbnv ypbnv added this to the 8.3.0 milestone Aug 26, 2019
@ypbnv ypbnv requested a review from garymathews August 26, 2019 15:35
@build build requested a review from a team August 26, 2019 15:56
@build
Copy link
Contributor

build commented Aug 26, 2019 •
edited
Loading

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.
Messages
📖

💾 Here's the generated SDK zipfile.
📖

✅ All tests are passing
Nice one! All 3899 tests are passing.
(There are 475 skipped tests not included in that total)
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Generated by 🚫 dangerJS against 3e25582

garymathews
garymathews approved these changes Aug 26, 2019
View reviewed changes
Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

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

CR: PASS

@sgtcoolguy sgtcoolguy changed the base branch from 8_3_X to master September 9, 2019 18:04
@sgtcoolguy sgtcoolguy changed the title fix(android): fix onlink callback being cretion only (8_3_X) fix(android): fix onlink callback being cretion only Sep 9, 2019
@sgtcoolguy sgtcoolguy self-requested a review September 9, 2019 19:21
sgtcoolguy
sgtcoolguy previously requested changes Sep 9, 2019
View reviewed changes
Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

I think we should add a simple unit test here to create a web view and set the onlink property after creation and verify it ends up getting called.

@sgtcoolguy sgtcoolguy dismissed their stale review September 10, 2019 13:36

I just realized that this requires interactive UI to click the link in the webview, so I suppose my review request to add a unit test for this makes no sense.

@ssjsamir ssjsamir self-requested a review September 10, 2019 15:28
@jquick-axway
Copy link
Contributor

@sgtcoolguy, @ypbnv,

This is in regards to unit testing. I remember looking into this about 8 years ago. On Android, the native listener only works for physical taps on links. That listener won't be invoked when attempting to navigate via JavaScript, such as by setting the window.location property. But that said, I do remember iOS' old UIWebView would invoke its "onlink" equivalent when navigating via JavaScript making it easy to unit test, but I don't know if the same is true with iOS' WKWebView.

I believe our Appium test suite can simulate physical touches. Perhaps this is the way to go for automated testing?

ssjsamir
ssjsamir approved these changes Sep 24, 2019
View reviewed changes
Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed: Able to see the "onlink" callback being invoked. Tested with the test case above.

Test Environment

MacOS Catalina 10.15 Beta
Node.js ^8.11.1
Android Pixel XL 7.1 Emulator 
"NPM":"4.2.15","CLI":"7.1.1"

@sgtcoolguy sgtcoolguy merged commit 3a46b79 into tidev:master Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garymathews garymathews garymathews approved these changes

@ssjsamir ssjsamir ssjsamir approved these changes

@sgtcoolguy sgtcoolguy sgtcoolguy left review comments

Assignees
No one assigned
Labels
android bug in-qe-testing 🕵 needs tests 🚨 parity 👯‍♀️
Projects
None yet
Milestone
8.3.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ypbnv @build @jquick-axway @sgtcoolguy @garymathews @ssjsamir