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

Prevent accidental clicks outside editor with active text edit from closing note #228

Open
KennySmash opened this issue May 17, 2022 · 9 comments
Labels
v2.x Legacy 2.x relesae

Comments

@KennySmash
Copy link

Having an issue where some users are creating multiple paragraph notes and accidentally click out the editor which closes and they lose their notes. could there be an option or someway to forward the click events so we can run app-side logic to reject the event if conditions are met

@rsimon
Copy link
Member

rsimon commented Aug 18, 2022

Hi,

sorry this dropped off the radar. I wonder if a better behavior wouldn't be to generally disable the current behavior, which is to consider a click outside the annotation a "cancel" event. In this case, only the Cancel button would close the popup, clicking outside would do nothing.

In read-only mode, on the other hand, clicking outside could close the popup. (Not 100% sure though, because it would introduce a bit of inconsistency between read- and readonly-mode.)

I'm also linking to a related issue here for RecogitoJS:
recogito/recogito-js#79

@KennySmash
Copy link
Author

No worries, I've actually managed to "fix" this by removing the event on the core lib and then building the whole lot from source,
I don't think it would be a useful PR since its pretty specific but with thinking about it, my ideal would be having each event exposed and then if we needed to overwrite the default behaviour then we can pass in functions for an event which will allow us to prevent if needed. maybe for the future we could also have a set of options to control what buttons appear in the button row

I agree that the default behaviour should be consistent

@rsimon
Copy link
Member

rsimon commented Aug 20, 2022

Ah, interesting to hear! I haven't really thought about the possibility of making events cancel-able. TBH I could imagine this to get fairly complicated. Did you end up implementing something like this? (For specific events?) Or really just removed the events from source you didn't need?

FWIW: I'm starting to work on Annotorious V3, and one of the major changes is that annotation layer/shape rendering will be completely de-coupled from the editor popup. Essentially, "headless mode" would be the normal behavior of the base library. And then there will be an "editor add-on" that provides the identical functionality (and design) as the current version of Annotorious. However, the idea would be that folks could just drop the default editor, and implement whatever popup they need themselves. In many cases, I think this will be much easier than forcing people to stick to a specific "widget plugin API" as it's the case now.

@KennySmash
Copy link
Author

So I just removed them outright from the core but I was thinking of how to handle the events and you could do via a callback,

so if anno.on('some-evt', (eventDetails, evt-in, evt-out)=>{ . . . }) would be possible so we had the chance to collect or transform the event before its submitted to the internal event stack, I think it would match the arch of v3 well with allowing freedom to plug in other systems at will, but all events even internal ones would need to be exposed first

@rlskoeser
Copy link

rlskoeser commented Oct 20, 2022

@rsimon I have a similar problem with trying to optionally cancel the cancelSelected event. We're running annotorious in headless mode with our own editor (annotorious-tahqiq), and users were accidentally canceling the editor and losing changes. I have a partial fix, but it's turning out more complicated than I thought and still doesn't handle all cases. Wondering if you could advise on a better approach?

My current solution is to check if the active editor has changes, then prompt the user to confirm the cancel or continue editing. Since there's no way to stop the annotorious cancel event, I'm reselecting the current annotation (with a flag to skip the cancel confirmation, since selecting seems to fire the cancel event). This seems to work ok for existing annotations, but doesn't work for a new annotation because there's no id to reselect it with. Here's the relevant portion of my cancel signal handler.

Maybe there's a simpler approach that would prevent the problem before it gets to this point?

@rsimon
Copy link
Member

rsimon commented Oct 22, 2022

Hi @rlskoeser, TBH I'm not really a fan of the pattern of intercepting and blocking events. (My feeling is that this might inadvertadly break other parts of an application; people will need to start thinking the order by which they've registered their event handlers and so on.)

That being said: making click outside equivalent to Cancel probably wasn't the best decision in Annotorious' design. But it will have to stay that way for the time being (= until Annotorious v3 gets ready for prime time). As a workaround, what might be maybe a bit more robust than restoring the annotation (but still hugely hacky!) would be to

  • listen to annotation select events
  • block mouse events to the annotation canvas by temporarily setting a pointer-events: none CSS property on the Annotorious SVG element.

This way, no cancel event would ever get generated if the user clicks the image. Once the editor closes, you'd need to restore the pointer-events state back to all so that draw/select would work again.

@rlskoeser
Copy link

@rsimon thanks for the suggestion, that sounds like a much better approach — I'd much rather avoid the cancel event than try to catch it and stop it (which doesn't work completely and is more complicated). The only other case I'm aware of is if you click out of the editor and hit the escape key — do you know of an easy way I can disable that while the annotation is being edited? (That's a bit more deliberate so maybe not so much of a problem, but thought I'd ask.)

@rsimon
Copy link
Member

rsimon commented Oct 24, 2022

@rlskoeser phew... good question. Well, one wild approach would be to not use the pointer-events: none trick above. Instead, insert a custom DIV in front of the annotation layer, with position: absolute, top and left 0, width and height 100%, so that it completely covers the annotation layer. And then toggle this on and off.

This way, clicks outside the editor wouldn't go through. Once the user clicks the the cover DIV, you would also get the keyboard events. (You might have to give it a tabindex attribute, I think, to make it focusable. And you'll almost certainly need to give it background-color: transparent so that it receives events.)

It is quite a hack. You'd need to play a bit to find out where in the DOM the DIV should go exactly, and you might have to register event listeners that cancel the events as they pass through... But it should be feasible in principle.

@rlskoeser
Copy link

Whew, yeah. That sounds pretty hacky and would also prevent zooming or panning around the image while transcribing, so I'm not going to go down that path. I'll just work on the simpler option you suggested, and add a prompt to the cancel button in our editor to confirm cancellation if there are unsaved edits.

Thanks for helping with ideas!

@rsimon rsimon added the v2.x Legacy 2.x relesae label Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.x Legacy 2.x relesae
Projects
None yet
Development

No branches or pull requests

3 participants