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

Correctly assign click event target in shadow dom #7357

Merged
merged 5 commits into from
Feb 17, 2025

Conversation

alexcjohnson
Copy link
Collaborator

Fixes #6108

When you start a drag - which happens on mouse down, even before you actually move the mouse - we attach a "coverslip" element to document.body, covering the entire page, so that we can continue tracking mouse movements outside the graph while the mouse is down. This is useful (and expected by users) for panning and sometimes zooming, the drag shouldn't stop when the mouse leaves the graph. We could look into whether this is still necessary in today's browsers, which have gotten more sophisticated about targeting events, but it was clearly required in the early days of plotly.js. Anyway the problem comes when the graph is in a shadow DOM, because then it's not really allowed access to document.body. The net effect of this is events that had been targeted at elements inside the graph, after the browser sees that they're being handled by the coverslip, they look like they're targeted at the shadow DOM container element instead, and when we use this element for position reference instead of the original target, we get the wrong coordinates so we select the wrong data point.

I fixed this by packaging the original event into a plain object instead of a MouseEvent, ensuring the correct target element is included, and passing that along to our handlers.

Adapted from @slietar's codepen, here's the code I've been using in the test dashboard to test this fix:

(() => {
let plots = document.getElementById('plots');
try { plots.removeChild(document.getElementById('graph2')); } catch(e) {}
let graph = document.getElementById('graph');
let graph2 = document.createElement('div');
graph2.id = 'graph2';
plots.appendChild(graph2);
let n = 100;
let fill = (n, fn) => new Array(n).fill(0).map(() => fn());

// Normal distribution
function normal() {
  return Math.sqrt(-2.0 * Math.log(Math.random())) * Math.cos(2.0 * Math.PI * Math.random());
}

// Initialize the shadow root

let root = graph2.attachShadow({ mode: 'open' });
let rootPlot = document.createElement('div');
root.appendChild(rootPlot);

let trace = {x: fill(n, normal), y: fill(n, normal), mode: 'markers' };
Plotly.newPlot(graph, [{...trace}], {width: 500, height: 500});
Plotly.newPlot(rootPlot, [{...trace}], {width: 500, height: 500});

// Add Plotly stylesheets to the shadow root

let style = document.createElement('style');
root.appendChild(style);

for (let plotlyStyle of document.querySelectorAll('[id^="plotly.js-"]')) {
  for (let rule of plotlyStyle.sheet.rules) {
    style.sheet.insertRule(rule.cssText);
  }
}
})();

While I was at it I removed an obsolete fallback for creating new synthetic mouse events when new MouseEvent fails. This appears to have ~10 years of support.

and remove obsolete API for creating new synthetic mouse events
@alexcjohnson alexcjohnson added the bug something broken label Feb 8, 2025
@@ -1059,17 +1060,17 @@ describe('Test click interactions:', function() {
width: 600,
height: 600
}).then(function() {
expect(gd.layout.xaxis.range).toBeCloseToArray([1, 2.068]);
expect(gd.layout.xaxis.range).toBeCloseToArray([1, 2.068], 1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I slightly loosened this test, then this whole file passed locally. I had to look it up, apparently "1" as the second arg means a range of 10^-1, ie a span of 0.1, ie +/- 0.05, whereas the default is +/- 0.005 and I was getting errors of ~0.007.

@gvwilson gvwilson requested a review from emilykl February 10, 2025 14:50
@gvwilson gvwilson added P1 needed for current cycle fix fixes something broken and removed bug something broken labels Feb 10, 2025
Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @alexcjohnson ! 🚀

@alexcjohnson alexcjohnson merged commit 93f761f into master Feb 17, 2025
1 check passed
@alexcjohnson alexcjohnson deleted the alex/shadow-click branch February 17, 2025 21:19
@slietar
Copy link

slietar commented Mar 4, 2025

Thanks @alexcjohnson!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix fixes something broken P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect coordinates on click in scatter plot inside shadow dom
4 participants