brunob / leaflet.fullscreen

Leaflet.Control.FullScreen for Leaflet
https://brunob.github.io/leaflet.fullscreen/
MIT License
376 stars 107 forks source link

Fix missing screenfull check in onRemove and _createButton #99

Closed pfrumau closed 2 years ago

pfrumau commented 2 years ago

I get an error on ios because screenful is not supported:

in this._screenfull.raw.fullscreenchange the raw variable is undefined and therefore the app crashes.

Fix: add undefined check before handlers: if (this._screenfull.raw)

l.238

        onRemove: function () {
            leaflet.DomEvent
                .off(this.link, 'click', leaflet.DomEvent.stopPropagation)
                .off(this.link, 'click', leaflet.DomEvent.preventDefault)
                .off(this.link, 'click', this.toggleFullScreen, this);

            if (this._screenfull.raw) {
                leaflet.DomEvent
                    .off(this._container, this._screenfull.raw.fullscreenchange, leaflet.DomEvent.stopPropagation)
                    .off(this._container, this._screenfull.raw.fullscreenchange, leaflet.DomEvent.preventDefault)
                    .off(this._container, this._screenfull.raw.fullscreenchange, this._handleFullscreenChange, this);

                leaflet.DomEvent
                    .off(document, this._screenfull.raw.fullscreenchange, leaflet.DomEvent.stopPropagation)
                    .off(document, this._screenfull.raw.fullscreenchange, leaflet.DomEvent.preventDefault)
                    .off(document, this._screenfull.raw.fullscreenchange, this._handleFullscreenChange, this);
            }
        },

l.256

        _createButton: function (title, className, content, container, fn, context) {
            this.link = leaflet.DomUtil.create('a', className, container);
            this.link.href = '#';
            this.link.title = title;
            this.link.innerHTML = content;

            this.link.setAttribute('role', 'button');
            this.link.setAttribute('aria-label', title);

            leaflet.DomEvent
                .on(this.link, 'click', leaflet.DomEvent.stopPropagation)
                .on(this.link, 'click', leaflet.DomEvent.preventDefault)
                .on(this.link, 'click', fn, context);

            if (this._screenfull.raw) {
                leaflet.DomEvent
                    .on(container, this._screenfull.raw.fullscreenchange, leaflet.DomEvent.stopPropagation)
                    .on(container, this._screenfull.raw.fullscreenchange, leaflet.DomEvent.preventDefault)
                    .on(container, this._screenfull.raw.fullscreenchange, this._handleFullscreenChange, context);

                leaflet.DomEvent
                    .on(document, this._screenfull.raw.fullscreenchange, leaflet.DomEvent.stopPropagation)
                    .on(document, this._screenfull.raw.fullscreenchange, leaflet.DomEvent.preventDefault)
                    .on(document, this._screenfull.raw.fullscreenchange, this._handleFullscreenChange, context);
            }

            return this.link;
        },
pfrumau commented 2 years ago

i have a commit ready but no access to repo

brunob commented 2 years ago

I get an error on ios because screenful is not supported

Are you usong iOS ?

i have a commit ready but no access to repo

you have to push a pull request

RoboVij commented 2 years ago

@brunob I'm getting following error when this plugin is used in typescript.

TypeError: Cannot read properties of undefined (reading 'fullscreenchange')
    at NewClass._createButton (uic-map.entry.js:512:41)
    at NewClass.onAdd

I installed the types and screenfull plugin as well.

When I tried the above @pfrumau suggested code, by replacing it directly in the nodemodules' Control.Fullscreen.js file, the error is gone. But the fullscreen button is not appearing properly and not working when clicked.

I'm using Windows and

"leaflet.fullscreen": "^2.2.0",
"@types/leaflet.fullscreen": "^1.6.1",
"screenfull": "^6.0.1"

InkedScreenshot 2022-02-01 151235

pfrumau commented 2 years ago

the icon not showing has to do with the CSS not being loaded correctly. maybe you missed the fontawesome in headers / css loader? functionality im not so sure

RoboVij commented 2 years ago

Thanks for the tip. I'm using this plugin in Stencil.js, which has a specific way of importing the css files. Just got to know about that. Functionality is working now, but the icon is still not appearing. Stencil has some issues showing SVGs.

But ya it's only working with your above changes. So the plugin should be updated with your changes.

sfusato commented 2 years ago

Ran into the same bug on iPhone:

TypeError: undefined is not an object (evaluating 'this._screenfull.raw.fullscreenchange')

What's really strange is that the demo itself works fine on the iPhone.

ps: submitted a PR using @pfrumau suggestion fix.