Raruto / leaflet-elevation

Leaflet plugin that allows to add elevation profiles using d3js
GNU General Public License v3.0
252 stars 82 forks source link

Misalignment of chart profile when `preferCanvas: true` on MacOS #232

Open hupe13 opened 1 year ago

hupe13 commented 1 year ago

Hi Raruto,

testing the pull request regarding gesture handling, I found out, that a similar SVG issue like this exists on MacOS Chrome 110 and Safari if:

preferCanvas = true

Chrome:

svg issue

Safari:

Bildschirmfoto 2023-02-22 um 12 10 07

(sorry about editing, I forgot to note my relavant setting).

Raruto commented 1 year ago

Hi @hupe13,

so I guess the solution may be similar / related to: https://github.com/Raruto/leaflet-elevation/commit/b16c06ddd311bdd46f34a04989e0cb128c84aeae

In case you feel like doing some debugging, I suggest you to use browserstack local (if you don't already have an account 👉 https://www.browserstack.com/open-source)

👋 Raruto

hupe13 commented 1 year ago

https://github.com/Raruto/leaflet-elevation/blob/868179234d8e9b8f7beaea57d0e34d8bd79a2622/src/components/d3.js#L613 This line must be

if(/Mac|iPod|iPhone|iPad/.test(navigator.platform) && /AppleWebkit/i.test(navigator.userAgent) && !/Chrome/.test(navigator.userAgent)) {

then Chrome works.

In Safari whithout any transformation console.log(canvas) shows, that canvas on the right position and size, but the diagram is to big and not in the canvas area. And the z-index is wrong.

if(/Mac|iPod|iPhone|iPad/.test(navigator.platform) && /AppleWebkit/i.test(navigator.userAgent) && !/Chrome/.test(navigator.userAgent)) {
       //canvas   .style("transform", "translate(" + margins.left + "px," + margins.top + "px)");
       console.log(canvas);
}

grafik

I can't get it in the right position.

Raruto commented 1 year ago

@hupe13 Last night I did some local tests within the /test folder (with browserstack + MacOS Ventura) and it seemed to work correctly.

Do you have a particular test page of yours that is giving you problems? Perhaps are we testing different leaflet or leaflet-elevation versions?

👋 Raruto

hupe13 commented 1 year ago

You can see the problem on my page.

I have an error handler for the problem, it is currently turned off.

Raruto commented 1 year ago

@hupe13 Did you test prefercanvas: true without wordpress as well?

From what I see it could also be that:

Below you can see how your test-mit-wp.gpx file looks to me if update the /test folder accordingly:

Mac OS Ventura + Chrome 110 + leaflet 1.9.3

image

Mac OS Ventura + Safari 16 + leaflet 1.9.3

image

👋 Raruto

hupe13 commented 1 year ago

You are right, it is my problem, your example works. It could be that it is related to leaflet-ui, I don't use that. I am looking and will report.

Sorry and thank you for you help.

hupe13 commented 1 year ago

I have been able to narrow down the problem further but have not yet solved it. Your leaflet-elevation_extended-ui.html has this as well: Bildschirmfoto 2023-02-24 um 12 24 25

hupe13 commented 1 year ago

The reason was a css, I don't remember why I configured that. Probably updates from leaflet-elevation made it obsolete. I don't know. But the above problem still exists.

And you should change the line 613 (only this line) for Chrome:

if(/Mac|iPod|iPhone|iPad/.test(navigator.platform) && /AppleWebkit/i.test(navigator.userAgent) && !/Chrome/.test(navigator.userAgent)) {
Raruto commented 1 year ago

The reason was a css, I don't remember why I configured that. Probably updates from leaflet-elevation made it obsolete. I don't know.

Don't keep me on my toes, i'm a curious guy! Which css rule? 🌵

And you should change the line 613 (only this line) for Chrome:

if(/Mac|iPod|iPhone|iPad/.test(navigator.platform) && /AppleWebkit/i.test(navigator.userAgent) && !/Chrome/.test(navigator.userAgent)) {

Ok, I'll let you know when I've done some testing

👋 Raruto

hupe13 commented 1 year ago

hupe13/extensions-leaflet-map-github/css/leafext.css#L38-L40

Raruto commented 1 year ago

I have been able to narrow down the problem further but have not yet solved it. Your leaflet-elevation_extended-ui.html has this as well:

Bildschirmfoto 2023-02-24 um 12 24 25

You should change the line 613 (only this line) for Chrome:

if(/Mac|iPod|iPhone|iPad/.test(navigator.platform) && /AppleWebkit/i.test(navigator.userAgent) && !/Chrome/.test(navigator.userAgent)) {

@hupe13 Am I missing something?, here's how I see it without changing anything in code:

image

hupe13 commented 1 year ago
console.log(navigator.userAgent);

Safari logs:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/15.6.1 Safari/605.1.15

Chrome logs:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/110.0.0.0 Safari/537.36

leaflet-elevation_extended-ui.html is displayed correctly in Chrome without any changes. My example isn't.

I do not know what to do.

Raruto commented 1 year ago

@hupe13 if you are referring to the z-index (chart tooltip) take a look at this too: Understanding z-index - The stacking context

If you link me again to the page that's giving you trouble, I'll look at it later too.

👋 Raruto

Raruto commented 1 year ago

@hupe13 ref: https://github.com/Raruto/leaflet-elevation/issues/232#issuecomment-1444050340

My fault, if you look closely at my image there is a horizontal shift in my example too.


I'm no expert on the subject but I imagine that by now the chrome developers have fixed the following bug:

Anyway, I think the solution you proposed works correctly on Mac OS Chrome desktop, but it should also be tested on ios (chrome and mobile safari) before proceeding.

As for https://github.com/Raruto/leaflet-elevation/issues/232#issuecomment-1443554554, it seems me to be an issue related only to Safari browsers (if someone sends me a Mac I can also investigate better .. 😋).

👋 Raruto

hupe13 commented 1 year ago

I don't have an iPhone also. I do not invest any more time at the moment, with the change of the line 613 and set preferCanvas to false on Safari it works fine on MacOS, and on Chrome also.

https://github.com/hupe13/extensions-leaflet-map-github/blob/4b3288ed2b3f9f1e21e7a5bb45d37f325057d2cf/php/elevation.php#L744-L749

Raruto commented 1 year ago

@hupe13 continue here from https://github.com/Raruto/leaflet-elevation/pull/257#discussion_r1197201932

do you confirm that this is the patch you proposed in here? 👉 https://github.com/Raruto/leaflet-elevation/issues/232

Yes it works right now on Chrome and Safari.

But this problem still exists on Safari: grafik This is your /examples/leaflet-elevation.html. I set preferCanvas: true,.

Here's the reason behind this bug

And here's how we might go about fixing it (not perfect, styles ? 🆘):

// Attempt to fix: https://bugs.webkit.org/show_bug.cgi?id=201110
function fixingWebkitBug201110({chart, panes, canvas, foreignObject}) {
  if (
    /Mac|iPod|iPhone|iPad/.test(navigator.platform) &&
    /AppleWebkit/i.test(navigator.userAgent) &&
    !/Chrome/.test(navigator.userAgent)
  ) {
    panes.tooltip.attr('id', 'webkit-' + Math.random().toFixed(20).slice(2));
    canvas.attr('style', canvas.attr('style')+ ';position: absolute; inset: 0; z-index:0;');
    foreignObject
    .append("svg:svg")
    .attr('style', 'position: absolute; inset: 0; width: 100%; height: 100%; z-index:1; stroke: #fff;')
    .html('<use href="#' + panes.tooltip.attr('id') + '" style="" />');
  }
}

// The following code is needed just to make you able to test it (without changing the source..)

// Hook "fixingWebkitBug201110" into class constructor
L.Control.Elevation.addInitHook(function() {
  this.on('elechart_init', function() {
    const chart = this._chart._chart;
    fixingWebkitBug201110({
      chart: chart,
      panes: chart.panes,
      canvas: chart.utils.canvas,
      foreignObject: chart.utils.canvas.select(function() { return this.parentNode; }
    });
  });
});

👋 Raruto