[CB-12266] InappBrowser: Browser: loadstop event.url is not a string - ASF JIRA

Details

Type: Bug
Status: Resolved
Priority: Minor
Resolution: Fixed
Affects Version/s: 3.5.0
Fix Version/s: None
Component/s: cordova-browser, cordova-plugin-inappbrowser
Labels:
- pluginsrelease
Environment:

All browsers (Chrome, Firefox and IE tested)

Description

In the browser, the url attribute of the loadstop event (InAppBrowserEvent) is not a string as documented, but a Location object. The url is stored in event.href.

Steps to reproduce:

$ cordova create eventstop_url
$ cd eventstop_url
$ cordova plugin add cordova-plugin-inappbrowser

Run the following code after device ready:

// https://ssl.gstatic.com as an example because it is allowed by the default CSP
var ref = cordova.InAppBrowser.open('https://ssl.gstatic.com', '_blank');
ref.addEventListener('loadstop', function (event) {
    console.log("loadstop event:", event);
});

$ cordova serve

The console output is:

loadstop event: Object {type: "loadstop", url: Location}

Issue Links

links to

GitHub Pull Request #217

GitHub Pull Request #218

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

ASF GitHub Bot added a comment - 13/Apr/17 12:48

GitHub user matrosov-nikita opened a pull request:

https://github.com/apache/cordova-plugin-inappbrowser/pull/217

~~CB-12266~~ browser: Fix type for InAppBrowserEvent url in docs

<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist
is intended as a quick reference, for complete details please see our Contributor Guidelines:

http://cordova.apache.org/contribute/contribute_guidelines.html

Thanks!
-->

1. 1. Platforms affected
    browser

1. 1. What does this PR do?
    fix doc's description for URL

1. 1. Checklist

[x] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
[x] Commit message follows the format: "~~CB-3232~~: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
[ ] Added automated test coverage as appropriate for this change.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/matrosov-nikita/cordova-plugin-inappbrowser fix-docs

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cordova-plugin-inappbrowser/pull/217.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #217

commit 6a93e34d459cd13ce74c9b01b595e242399ece16
Author: Nikita Matrosov <v-nimatr@microsoft.com>
Date: 2017-04-06T10:33:30Z

~~CB-12266~~ browser: Fix type for InAppBrowserEvent url in docs

Show

ASF GitHub Bot added a comment - 13/Apr/17 12:48 GitHub user matrosov-nikita opened a pull request: https://github.com/apache/cordova-plugin-inappbrowser/pull/217 CB-12266 browser: Fix type for InAppBrowserEvent url in docs  Platforms affected browser What does this PR do? fix doc's description for URL Checklist [x] [Reported an issue] ( http://cordova.apache.org/contribute/issues.html ) in the JIRA database [x] Commit message follows the format: " CB-3232 : (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected. [ ] Added automated test coverage as appropriate for this change. You can merge this pull request into a Git repository by running: $ git pull https://github.com/matrosov-nikita/cordova-plugin-inappbrowser fix-docs Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-plugin-inappbrowser/pull/217.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #217 commit 6a93e34d459cd13ce74c9b01b595e242399ece16 Author: Nikita Matrosov <v-nimatr@microsoft.com> Date: 2017-04-06T10:33:30Z CB-12266 browser: Fix type for InAppBrowserEvent url in docs

Hide

Permalink

ASF GitHub Bot added a comment - 13/Apr/17 13:28

Github user cordova-qa commented on the issue:

https://github.com/apache/cordova-plugin-inappbrowser/pull/217

Cordova CI Build has one or more failures.

*Commit* - [Link](https://github.com/apache/cordova-plugin-inappbrowser/pull/217/commits/6a93e34d459cd13ce74c9b01b595e242399ece16)
*Dashboard* - [Link](http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/137/)

36 tests run, 0 skipped, 0 failed.

Show

ASF GitHub Bot added a comment - 13/Apr/17 13:28 Github user cordova-qa commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/217 Cordova CI Build has one or more failures. * Commit * - [Link] ( https://github.com/apache/cordova-plugin-inappbrowser/pull/217/commits/6a93e34d459cd13ce74c9b01b595e242399ece16 ) * Dashboard * - [Link] ( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/137/ ) 36 tests run, 0 skipped, 0 failed.

Hide

Permalink

ASF GitHub Bot added a comment - 13/Apr/17 13:48

Github user alsorokin commented on the issue:

https://github.com/apache/cordova-plugin-inappbrowser/pull/217

Let there be tests

Show

ASF GitHub Bot added a comment - 13/Apr/17 13:48 Github user alsorokin commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/217 Let there be tests

Hide

Permalink

ASF GitHub Bot added a comment - 13/Apr/17 14:24

Github user cordova-qa commented on the issue:

https://github.com/apache/cordova-plugin-inappbrowser/pull/217

Cordova CI Build has one or more failures.

36 tests run, 0 skipped, 0 failed.

Show

ASF GitHub Bot added a comment - 13/Apr/17 14:24 Github user cordova-qa commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/217 Cordova CI Build has one or more failures. * Commit * - [Link] ( https://github.com/apache/cordova-plugin-inappbrowser/pull/217/commits/6a93e34d459cd13ce74c9b01b595e242399ece16 ) * Dashboard * - [Link] ( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/138/ ) 36 tests run, 0 skipped, 0 failed.

Hide

Permalink

ASF GitHub Bot added a comment - 13/Apr/17 14:38

Github user alsorokin commented on the issue:

https://github.com/apache/cordova-plugin-inappbrowser/pull/217

Let there be tests

Show

ASF GitHub Bot added a comment - 13/Apr/17 14:38 Github user alsorokin commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/217 Let there be tests

Hide

Permalink

ASF GitHub Bot added a comment - 13/Apr/17 15:10

Github user cordova-qa commented on the issue:

https://github.com/apache/cordova-plugin-inappbrowser/pull/217

Cordova CI Build has completed successfully.

42 tests run, 0 skipped, 0 failed.

Show

ASF GitHub Bot added a comment - 13/Apr/17 15:10 Github user cordova-qa commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/217 Cordova CI Build has completed successfully. * Commit * - [Link] ( https://github.com/apache/cordova-plugin-inappbrowser/pull/217/commits/6a93e34d459cd13ce74c9b01b595e242399ece16 ) * Dashboard * - [Link] ( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/139/ ) 42 tests run, 0 skipped, 0 failed.

Hide

Permalink

Pieter-Jan Dewitte added a comment - 13/Apr/17 21:06

Thanks for the interest in this issue.

I'm surprised with the proposed resolution in the pull request (changing the documentation): this creates a difference between the browser platform and the other platforms. I would think that differences between platforms are best avoided.

Is there a reason why it would be desirable to have the Location object rather than the url string on the browser platform, knowing it's not available on the other platforms?

Show

Pieter-Jan Dewitte added a comment - 13/Apr/17 21:06 Thanks for the interest in this issue. I'm surprised with the proposed resolution in the pull request (changing the documentation): this creates a difference between the browser platform and the other platforms. I would think that differences between platforms are best avoided. Is there a reason why it would be desirable to have the Location object rather than the url string on the browser platform, knowing it's not available on the other platforms?

Hide

Permalink

Filip Maj added a comment - 24/Apr/17 23:06

I agree with Pieter-Jan Dewitte - the proper solution is to align the browser platform.

Show

Filip Maj added a comment - 24/Apr/17 23:06 I agree with Pieter-Jan Dewitte - the proper solution is to align the browser platform.

Hide

Permalink

ASF GitHub Bot added a comment - 25/Apr/17 23:21

GitHub user filmaj opened a pull request:

https://github.com/apache/cordova-plugin-inappbrowser/pull/218

~~CB-12266~~: (browser platform) loadstop event.url is now a string

… instead of an object, aligning it with the other platforms.

1. 1. Platforms affected

Browser.

1. 1. What does this PR do?

Fixes ~~CB-12266~~(https://issues.apache.org/jira/browse/CB-12266): previously in the browser platform, `loadstop` event object's `url` property would be an object and not a string, like the other platforms.

1. 1. What testing has been done on this change?

Tested in Chrome on a Mac desktop.

1. 1. Checklist

[x] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database: ~~CB-12266~~(https://issues.apache.org/jira/browse/CB-12266)
[x] Commit message follows the format: "~~CB-3232~~: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
[ ] Added automated test coverage as appropriate for this change.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/filmaj/cordova-plugin-inappbrowser ~~CB-12266~~

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cordova-plugin-inappbrowser/pull/218.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #218

commit bfb33988be6dae6708a254872da3bf4081eeeb7b
Author: filmaj <maj.fil@gmail.com>
Date: 2017-04-25T23:19:48Z

~~CB-12266~~: (browser platform) loadstop event.url is now a string instead of an object, aligning it with the other platforms.

Show

ASF GitHub Bot added a comment - 25/Apr/17 23:21 GitHub user filmaj opened a pull request: https://github.com/apache/cordova-plugin-inappbrowser/pull/218 CB-12266 : (browser platform) loadstop event.url is now a string … instead of an object, aligning it with the other platforms. Platforms affected Browser. What does this PR do? Fixes CB-12266 ( https://issues.apache.org/jira/browse/CB-12266): previously in the browser platform, `loadstop` event object's `url` property would be an object and not a string, like the other platforms. What testing has been done on this change? Tested in Chrome on a Mac desktop. Checklist [x] [Reported an issue] ( http://cordova.apache.org/contribute/issues.html ) in the JIRA database: CB-12266 ( https://issues.apache.org/jira/browse/CB-12266 ) [x] Commit message follows the format: " CB-3232 : (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected. [ ] Added automated test coverage as appropriate for this change. You can merge this pull request into a Git repository by running: $ git pull https://github.com/filmaj/cordova-plugin-inappbrowser CB-12266 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-plugin-inappbrowser/pull/218.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #218 commit bfb33988be6dae6708a254872da3bf4081eeeb7b Author: filmaj <maj.fil@gmail.com> Date: 2017-04-25T23:19:48Z CB-12266 : (browser platform) loadstop event.url is now a string instead of an object, aligning it with the other platforms.

Hide

Permalink

ASF GitHub Bot added a comment - 25/Apr/17 23:22

Github user filmaj commented on the issue:

https://github.com/apache/cordova-plugin-inappbrowser/pull/217

Please close this pull request - I have a PR to fix the underlying issue open over at #218.

Show

ASF GitHub Bot added a comment - 25/Apr/17 23:22 Github user filmaj commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/217 Please close this pull request - I have a PR to fix the underlying issue open over at #218.

Hide

Permalink

ASF GitHub Bot added a comment - 25/Apr/17 23:42

Github user cordova-qa commented on the issue:

https://github.com/apache/cordova-plugin-inappbrowser/pull/218

Cordova CI Build has completed successfully.

*Commit* - [Link](https://github.com/apache/cordova-plugin-inappbrowser/pull/218/commits/bfb33988be6dae6708a254872da3bf4081eeeb7b)
*Dashboard* - [Link](http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/142/)

36 tests run, 0 skipped, 0 failed.

Show

ASF GitHub Bot added a comment - 25/Apr/17 23:42 Github user cordova-qa commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/218 Cordova CI Build has completed successfully. * Commit * - [Link] ( https://github.com/apache/cordova-plugin-inappbrowser/pull/218/commits/bfb33988be6dae6708a254872da3bf4081eeeb7b ) * Dashboard * - [Link] ( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/142/ ) 36 tests run, 0 skipped, 0 failed.

Hide

Permalink

ASF subversion and git services added a comment - 25/Apr/17 23:45

Commit bfb33988be6dae6708a254872da3bf4081eeeb7b in cordova-plugin-inappbrowser's branch refs/heads/master from filmaj
[ https://git-wip-us.apache.org/repos/asf?p=cordova-plugin-inappbrowser.git;h=bfb3398 ]

~~CB-12266~~: (browser platform) loadstop event.url is now a string instead of an object, aligning it with the other platforms.

Show

ASF subversion and git services added a comment - 25/Apr/17 23:45 Commit bfb33988be6dae6708a254872da3bf4081eeeb7b in cordova-plugin-inappbrowser's branch refs/heads/master from filmaj [ https://git-wip-us.apache.org/repos/asf?p=cordova-plugin-inappbrowser.git;h=bfb3398 ] CB-12266 : (browser platform) loadstop event.url is now a string instead of an object, aligning it with the other platforms.

Hide

Permalink

ASF GitHub Bot added a comment - 25/Apr/17 23:45

Github user filmaj closed the pull request at:

https://github.com/apache/cordova-plugin-inappbrowser/pull/218

Show

ASF GitHub Bot added a comment - 25/Apr/17 23:45 Github user filmaj closed the pull request at: https://github.com/apache/cordova-plugin-inappbrowser/pull/218

Hide

Permalink

ASF GitHub Bot added a comment - 25/Apr/17 23:45

Github user filmaj commented on the issue:

https://github.com/apache/cordova-plugin-inappbrowser/pull/218

Fixed in bfb33988be6dae6708a254872da3bf4081eeeb7b

Show

ASF GitHub Bot added a comment - 25/Apr/17 23:45 Github user filmaj commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/218 Fixed in bfb33988be6dae6708a254872da3bf4081eeeb7b

Hide

Permalink

Filip Maj added a comment - 25/Apr/17 23:46

Fixed in https://github.com/apache/cordova-plugin-inappbrowser/commit/bfb33988be6dae6708a254872da3bf4081eeeb7b

Show

Filip Maj added a comment - 25/Apr/17 23:46 Fixed in https://github.com/apache/cordova-plugin-inappbrowser/commit/bfb33988be6dae6708a254872da3bf4081eeeb7b

Hide

Permalink

ASF GitHub Bot added a comment - 26/Apr/17 06:44

Github user matrosov-nikita closed the pull request at:

https://github.com/apache/cordova-plugin-inappbrowser/pull/217

Show

ASF GitHub Bot added a comment - 26/Apr/17 06:44 Github user matrosov-nikita closed the pull request at: https://github.com/apache/cordova-plugin-inappbrowser/pull/217

Hide

Permalink

ASF subversion and git services added a comment - 27/Apr/17 19:51

Commit bfb33988be6dae6708a254872da3bf4081eeeb7b in cordova-plugin-inappbrowser's branch refs/heads/1.7.x from filmaj
[ https://git-wip-us.apache.org/repos/asf?p=cordova-plugin-inappbrowser.git;h=bfb3398 ]

~~CB-12266~~: (browser platform) loadstop event.url is now a string instead of an object, aligning it with the other platforms.

Show

ASF subversion and git services added a comment - 27/Apr/17 19:51 Commit bfb33988be6dae6708a254872da3bf4081eeeb7b in cordova-plugin-inappbrowser's branch refs/heads/1.7.x from filmaj [ https://git-wip-us.apache.org/repos/asf?p=cordova-plugin-inappbrowser.git;h=bfb3398 ] CB-12266 : (browser platform) loadstop event.url is now a string instead of an object, aligning it with the other platforms.

People

Assignee:

Filip Maj

Reporter:

Pieter-Jan Dewitte

Votes:

0 Vote for this issue

Watchers:

4 Start watching this issue

Dates

Created:

18/Dec/16 22:30

Updated:

27/Apr/17 19:51

Resolved:

25/Apr/17 23:46

Development

Agile

View on Board