viritin / flow-viritin

Viritin inspired project for Vaadin Flow
Other
39 stars 15 forks source link

Geolocation ErrorListener: include "GeolocationPositionError.code" #48

Closed mvysny closed 3 months ago

mvysny commented 3 months ago

When the getCurrentLocation() call fails and GeolocationPositionError is thrown by JavaScript, Viritin calls the ErrorListener with the exception message. However, I'd argue that the "code" part is much more important:

  1. The "message" documentation says that "Returns a human-readable string describing the details of the error. Specifications note that this is primarily intended for debugging use and not to be shown directly in a user interface.". That means that I shouldn't show the message to the user; and since the message may differ from browser to browser, I can't even detect the type of the error from the message.
  2. Instead, the "code" field precisely identifies the kind of failure, and therefore I can show an informative localizable error message to the user, for example:

PERMISSION_DENIED -> The browser denied to provide the location. Please enable access to the location in your browser's settings.

POSITION_UNAVAILABLE -> Internal error obtaining the location, please try again.

TIMEOUT -> Could not obtain location because of timeout. Please try to move away from tall buildings, to a place with an unobstructed view of the sky.

I think the best way is to provide both the browserError and the code (int is enough, enum could be better). This unfortunately breaks backwards compatibility, but I'd argue that having the fix is more vital than backward compatibility in this case.

mvysny commented 3 months ago

To fix, you can patch Geolocation as follows:

        geolocation.geoerror = eventSourceElement.addEventListener("geoerror", e -> {
            final JsonObject detail = e.getEventData().getObject("event.detail");
            errorListener.geolocationError(((int) detail.getNumber("code")), detail.getString("message"));
            if(get) {
                geolocation.clearListeners();
            }
        });
        geolocation.geoerror.addEventData("event.detail");

and

                    "         e => {\n" +
                    "           const event = new CustomEvent('geoerror', {detail: {code: e.code, message: e.message}});\n" +
                    "           el.dispatchEvent(event);\n" +
                    "         },\n" +
mstahv commented 3 months ago

Made a slightly tuned version of your suggestion, error code now available via the event (which is passed ot the error listener now instead of the string). Most probably backwards compatible for most users.