Uploaded image for project: 'Apache Cordova'
  1. Apache Cordova
  2. CB-12266

InappBrowser: Browser: loadstop event.url is not a string

    Details

      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

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          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. Platforms affected
                browser
              1. What does this PR do?
                fix doc's description for URL
              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
          githubbot ASF GitHub Bot added a comment - 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! --> 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
          githubbot ASF GitHub Bot added a comment -

          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
          githubbot ASF GitHub Bot added a comment - 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
          githubbot ASF GitHub Bot added a comment -

          Github user alsorokin commented on the issue:

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

          Let there be tests

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

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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
          githubbot ASF GitHub Bot added a comment -

          Github user alsorokin commented on the issue:

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

          Let there be tests

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

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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
          pj.dewitte Pieter-Jan Dewitte added a comment -

          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
          pj.dewitte Pieter-Jan Dewitte added a comment - 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
          filmaj Filip Maj added a comment -

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

          Show
          filmaj Filip Maj added a comment - I agree with Pieter-Jan Dewitte - the proper solution is to align the browser platform.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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. Platforms affected

          Browser.

              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. What testing has been done on this change?

          Tested in Chrome on a Mac desktop.

              1. Checklist

          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
          githubbot ASF GitHub Bot added a comment - 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
          githubbot ASF GitHub Bot added a comment -

          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
          githubbot ASF GitHub Bot added a comment - 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
          githubbot ASF GitHub Bot added a comment -

          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
          githubbot ASF GitHub Bot added a comment - 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
          jira-bot ASF subversion and git services added a comment -

          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
          jira-bot ASF subversion and git services added a comment - 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
          githubbot ASF GitHub Bot added a comment -

          Github user filmaj closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user filmaj closed the pull request at: https://github.com/apache/cordova-plugin-inappbrowser/pull/218
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user filmaj commented on the issue:

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

          Fixed in bfb33988be6dae6708a254872da3bf4081eeeb7b

          Show
          githubbot ASF GitHub Bot added a comment - Github user filmaj commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/218 Fixed in bfb33988be6dae6708a254872da3bf4081eeeb7b
          Show
          filmaj Filip Maj added a comment - Fixed in https://github.com/apache/cordova-plugin-inappbrowser/commit/bfb33988be6dae6708a254872da3bf4081eeeb7b
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user matrosov-nikita closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user matrosov-nikita closed the pull request at: https://github.com/apache/cordova-plugin-inappbrowser/pull/217
          Hide
          jira-bot ASF subversion and git services added a comment -

          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
          jira-bot ASF subversion and git services added a comment - 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:
              filmaj Filip Maj
              Reporter:
              pj.dewitte Pieter-Jan Dewitte
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development