Fix DEL-key in better-polygon

annotorious / annotorious-v2-plugins

Plugins compatible with the RecogitoJS, Annotorious and AnnotoriousOSD annotation libraries

BSD 3-Clause "New" or "Revised" License

28 stars 20 forks source link

Fix DEL-key in better-polygon #27

Closed pwuertz closed 2 years ago

pwuertz commented 2 years ago

Attempt to fix some issues with the better-polygon plugin.

Fix DEL-key support in better-polygon by stopping key-event propagation on successful deletion of selected polygon points.

Fix double-click behavior by removing isTouch distinction (doesn't apply anymore?).

rsimon commented 2 years ago

Interesting - this PR didn't change anything for me. Problem #24 persists (polygon is still deleted completely on DEL).

rsimon commented 2 years ago

Ok, got it. It did work for me after I changed back the handler from the SVG element to document.body. I agree it's better to have the handler on the SVG element rather than the body. Right now this doesn't work, because Annotorious attaches the original DEL handler (which removes the annotation) to document.body. Therefore the cancellation fails. But I can change that.

Just one request: would it be possible to remove the changes to the onDblClick handler? As I wrote in the ticket: although the behavior differs from the "old" polygon plugin, this is the way it's supposed to work.

Cheers & thx, Rainer

pwuertz commented 2 years ago

Ok there seems to be a disagreement between different browser-engines on the event-handling order. The fix works for me in Chrome, but indeed with Firefox the whole polygon is still deleted. Did you test this with Firefox or are there possibly more factors affecting event handling?

(didn't check with Firefox before because I'm getting visual glitches with Annotorious on this browser)

pwuertz commented 2 years ago

Right, double-click-handling removed from this PR. Different topic.

rsimon commented 2 years ago

Hi,

weird. I tested both in Chrome and Firefox, and I'm seeing identical behavior in both browsers, although it's not exactly what I had expected:

attaching the handler to this.svg doesn't work for me. DEL deletes the entire polygon.
attaching the handler to document doesn't work either. I would have expected this to work, because the original handler that deletes the polygon is attached to document here.)
attaching the handler to document.body does work. The first delete handler is executed, points removed, and event propagation is stopped, so the polygon stays.

My assumption is that the event propagation is stopped on the way upwards from document.body to document. I don't know why it doesn't work for the SVG element. (In my case, I can say that the handler attached to the SVG element isn't getting called! Neither in Chrome nor FF.)

rsimon commented 2 years ago

Small update on this: it looks like I'm not seeing key events on the SVG element, because SVG isn't a focusable element per default.

If I make the SVG layer focusable by adding a tabindex attribute, the key handler starts working, and the event bubbling is properly canceled (both on Chrome + FF).

I wonder what the best approach would be now: we could either add tabindex to the Annotorious SVG layer, or keep attaching the handler to document.body and use an event delegation pattern to make sure the event is coming from this.svg?

pwuertz commented 2 years ago

Oh right, tabindex exists. Totally forgot to mention that I'm running everything on top of OpenSeadragon, which does indeed apply a tabindex somewhere in the hierarchy. Don't know if that might have had an effect.

Attaching the key-handlers to a tabindexed svg doesn't seem like a bad idea. Should work out of the box for multiple Annotorious instances per page as well. You wouldn't be able to DEL after shifting the focus away from the viewer but I don't think this would be too much of a surprise for a user.