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

setDiv() no longer detaches the map (since 4.15.x) #167

Closed
4 of 6 tasks
battika opened this issue Feb 2, 2019 · 16 comments
Closed
4 of 6 tasks

setDiv() no longer detaches the map (since 4.15.x) #167

battika opened this issue Feb 2, 2019 · 16 comments

Comments

@battika
Copy link

battika commented Feb 2, 2019

I'm submitting a ... (check one with "x")

  • question
  • any problem or bug report
  • feature request

If you choose 'problem or bug report', please select OS: (check one with "x")

  • Android
  • iOS
  • Browser

cordova information: (run $> cordova plugin list)

cordova-plugin-device 1.1.7 "Device"
cordova-plugin-googlemaps 2.5.0 "cordova-plugin-googlemaps"
cordova-plugin-ionic-webview 1.2.1 "cordova-plugin-ionic-webview"
cordova-plugin-splashscreen 4.1.0 "Splashscreen"
cordova-plugin-statusbar 2.4.1 "StatusBar"
cordova-plugin-whitelist 1.3.3 "Whitelist"
ionic-plugin-keyboard 2.2.1 "Keyboard"

If you use @ionic-native/google-maps, please tell the package.json (only @ionic-native/core and @ionic-native/google-maps are fine mostly)

    "@ionic-native/core": "4.20.0",
    "@ionic-native/google-maps": "4.20.0",
    "@ionic-native/splash-screen": "4.20.0",
    "@ionic-native/status-bar": "4.20.0",

Current behavior:
Starting with ionic native version 4.15, calling setDiv() or setDiv(null) will not detach the map from the currently attached HTML element but throws an error:

Cannot read property 'tagName' of undefined

According to the documentation omitting the argument should detach the map.

GoogleMap.prototype.setDiv = 
Changes the map div
@param **domNode** {HTMLElement | string} [options] 
If you want to display the map in an html element, you need to specify an element or id. 
If omit this argument, the map is detached from webview.

It is caused by the changes introduced in 4.15 to the setDiv() method.
Old, working setDiv() method:

/**
* Changes the map div
* @param domNode {HTMLElement | string} [options] If you want to display the map in an html element, you need to specify an element or id. If omit this argument, the map is detached from webview.
*/
@InstanceCheck()
setDiv(domNode?: HTMLElement | string): void {
if (typeof domNode === 'string') {
this._objectInstance.setDiv(document.querySelector('.show-page #' + domNode));
} else {
this._objectInstance.setDiv(domNode);
}
}

New, broken setDiv() method:
/**
* Changes the map div
* @param domNode {HTMLElement | string} [options] If you want to display the map in an html element, you need to specify an element or id. If omit this argument, the map is detached from webview.
*/
@InstanceCheck()
setDiv(domNode?: HTMLElement | string): void {
if (typeof domNode === 'string') {
(new Promise<any>((resolve, reject) => {
let i, targets: any[];
for (i = 0; i < TARGET_ELEMENT_FINDING_QUERYS.length; i++) {
targets = Array.from(document.querySelectorAll(TARGET_ELEMENT_FINDING_QUERYS[i] + domNode));
if (targets.length > 0) {
targets = targets.filter((target) => {
return !target.hasAttribute('__pluginmapid');
});
}
if (targets.length === 1 && targets[0] && targets[0].offsetWidth >= 100 && targets[0].offsetHeight >= 100) {
resolve(targets[0] as HTMLElement);
return;
}
}
reject('Can not find [#' + domNode + '] element');
}))
.then((element: HTMLElement) => {
this._objectInstance.setDiv(element);
});
} else {
if (domNode instanceof HTMLElement &&
!domNode.offsetParent &&
domNode.offsetWidth >= 100 && domNode.offsetHeight >= 100) {
this._objectInstance.setDiv(domNode);
} else {
throw new Error(domNode.tagName + ' is too small. Must be bigger than 100x100.');
}
}
}

In the new code when one attempts to call setDiv() with no parameters, it first checks whether string was passed, check fails, it then goes to the else branch where it checks whether HTML element is passed, check fails, it attempts to throw an exception:

throw new Error(domNode.tagName + ' is too small. Must be bigger than 100x100.');

And actually the error is caused by the exception handler because domNode has not tagName property as it is null.

So, a check for empty setDiv() value is missing for detaching the webview. I tried passing empty string but it did not help either.

@battika
Copy link
Author

battika commented Feb 4, 2019

@wf9a5m75 Masashi, if you want me to provide a sample project as well, please let me know, however it is about calling setDiv() on an active map.

@ChrisKemmerer
Copy link

I had the same problem. I modified the setDiv method to restore the check for an empty parameter. I do not have permission to submit a PR so here's my proposed change.

function (domNode) {
        var _this = this;

        if (!domNode) {
          this._objectInstance.setDiv(null);
        } else {
          if (typeof domNode === 'string') {
            (new Promise(function (resolve, reject) {
              var i, targets;
              for (i = 0; i < TARGET_ELEMENT_FINDING_QUERYS.length; i++) {
                targets = Array.from(document.querySelectorAll(TARGET_ELEMENT_FINDING_QUERYS[i] + domNode));
                if (targets.length > 0) {
                  targets = targets.filter(function (target) {
                    return !target.hasAttribute('__pluginmapid');
                  });
                }
                if (targets.length === 1 && targets[0] && targets[0].offsetWidth >= 100 && targets[0].offsetHeight >= 100) {
                  resolve(targets[0]);
                  return;
                }
              }
              reject('Can not find [#' + domNode + '] element');
            }))
              .then(function (element) {
                _this._objectInstance.setDiv(element);
              });
          } else {
            if (domNode instanceof HTMLElement &&
              !domNode.offsetParent &&
              domNode.offsetWidth >= 100 && domNode.offsetHeight >= 100) {
              this._objectInstance.setDiv(domNode);
            } else {
              throw new Error(domNode.tagName + ' is too small. Must be bigger than 100x100.');
            }
          }
        }
    };

@ChrisKemmerer
Copy link

BTW the getDiv function is non-functional too. Modified it to pass through to the cordova plugin.

function () {
        return this._objectInstance.getDiv();
    };

@battika
Copy link
Author

battika commented Feb 5, 2019

Thanks @ChrisKemmerer, good catch! Not sure why you cannot open a pull request, this option seems to be available for everyone.

@wf9a5m75
Copy link
Contributor

wf9a5m75 commented Feb 5, 2019

@battika
This is your code https://github.com/battika/ionic-gmaps-test
And it works as you expect with current latest versions.

167

@battika
Copy link
Author

battika commented Feb 6, 2019

Because it does not use setDiv() any longer neither getDiv(), it lets the plugin manage page transitions. Unfortunately it is not always possible but I manually have to manage visibility. For example when I have two maps in the app on two different root pages. In that case, if I want to preserve the states of the maps for faster loading, before I run NavController's setRoot() function I need to run setDiv() to hide the map and setDiv(id) to restore when I come back. This is one possible usage scenario but there are more. Anyway, I update my example to show you the issue.

@wf9a5m75
Copy link
Contributor

wf9a5m75 commented Feb 6, 2019

Please share the reproduce project files on GitHub repository

@battika
Copy link
Author

battika commented Feb 6, 2019

Updated this project:
https://github.com/battika/ionic-gmaps-test

Please test the project with both versions 4.14.0 and 4.20.0.

Results are below.

This is how it works with version 4.14.0 (as expected):

And with version 4.20.0:

@wf9a5m75
Copy link
Contributor

wf9a5m75 commented Feb 8, 2019

Thank you for providing. I will check this tomorrow.

@battika
Copy link
Author

battika commented Feb 9, 2019

Thank you

@wf9a5m75
Copy link
Contributor

Thank you. I fixed the problem.

@battika
Copy link
Author

battika commented Feb 10, 2019

Thank you, Masashi. Can I install this version to test or do I need to wait for the next, post 4.20.0 release?

@wf9a5m75
Copy link
Contributor

You can install this version.

git clone

npm install

npm run build

cd (your project for)

npm uninstall @ionic-native/core @ionic-native/google-maps

npm install (path to local ionic repo)/dist/@ionic-native/core

npm install (path to local ionic repo)/dist/@ionic-native/google-maps

@battika
Copy link
Author

battika commented Feb 14, 2019

Thank you :)

@battika
Copy link
Author

battika commented Feb 14, 2019

Donation sent

@wf9a5m75
Copy link
Contributor

Thank you!

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

Successfully merging a pull request may close this issue.

3 participants