ionic-team / ionic-native-google-maps

Google maps plugin for Ionic Native
Other
221 stars 125 forks source link

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

Closed battika closed 5 years ago

battika commented 5 years ago

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

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

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: https://github.com/ionic-team/ionic-native-google-maps/blob/c28d95e6a1576684d68d0dc6ca4f008c0959ed27/src/%40ionic-native/plugins/google-maps/index.ts#L2402-L2413 New, broken setDiv() method: https://github.com/ionic-team/ionic-native-google-maps/blob/edf5db62ee3468f82c1c04f44430236db8907122/src/%40ionic-native/plugins/google-maps/index.ts#L2734-L2770

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: https://github.com/ionic-team/ionic-native-google-maps/blob/edf5db62ee3468f82c1c04f44430236db8907122/src/%40ionic-native/plugins/google-maps/index.ts#L2767

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

function () {
        return this._objectInstance.getDiv();
    };
battika commented 5 years ago

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

wf9a5m75 commented 5 years ago

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

167

battika commented 5 years ago

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 commented 5 years ago

Please share the reproduce project files on GitHub repository

battika commented 5 years ago

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 commented 5 years ago

Thank you for providing. I will check this tomorrow.

battika commented 5 years ago

Thank you

wf9a5m75 commented 5 years ago

Thank you. I fixed the problem.

battika commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Thank you :)

battika commented 5 years ago

Donation sent

wf9a5m75 commented 5 years ago

Thank you!