Annotation Popup not showing sometimes (2 problems)

[henry-chris]

Go to your Demo website:

1. Make 4 annotations.
2. Select the top one from inside the NotesPanel. Popup appears.
3. Select the note below that one. Popup does not appear
4. Select the note below that one. Popup appears.
5. Select the note below that one (last one). Popup does not appear

2

We also have our own notespanel in our website. We do this when the note is clicked:

What seems to be happening with our app (only since updating to 5.2) is that:

1. Annot Selected event propagated to the popup
2. All annots are deselected
3. Specific annot is selected
4. handleClickOutside event triggers on the AnnotationPopup and closes the popup.

That's as far as I've gotten, am attempting to debug into it now. However, it prevents the popup from ever being shown when clicked from our notes panel. We aren't hooked into the 'react-onClickOutside' stuff. So maybe that's our problem?

[ZhijieZhang]

Hi Chris, I started to debug this issue right away after you posted, and I think I find the cause.

I'm going to give some background first to make sure that we are on the same page.

We made some significant changes to the notes panel in 5.2.1. In this patch version the notes panel is refactored to virtualize notes if there're more than 300 annotations in the document. This is to resolve an issue raised by a customer who reported that loading a document with 20k annotations will freeze the UI(because before, the UI will just mount 20k nodes in the DOM).

We also made another relatively big change to handle if a component should close when clicking outside of it. The motivation is #217 and another support ticket. The way we handled this before was to add some mouse handlers to the root element

https://github.com/PDFTron/webviewer-ui/blob/50866f847aa13348aeaf1b4d949633af28c55bfe/src/components/App/App.js#L57-L93

so that when we click somewhere in the app, the event will bubble up to the root element and the elements in the above code snippet will close. However this results in a problem, that is, if we don't want to close those elements when clicking on a specific element, we have to call e.stopPropagation() for that specific element to prevent the event from bubbling up. Just like you pointed out in #217, we call e.stopPropagation() in ToolButton for this reason. Doing this make the code overall a bit verbose and from a quick search in 5.1 we call e.stopPropagation in 28 files 35 times. We didn't really start working on changing this until a customer is reporting that when using mobile devices, moving a selected annotation around still keep the annotationPopup open and the style picker doesn't close when drawing an annotation in the document. The reason is that we didn't handle the touchStart case in the root element. But if we do that, we need to have more e.stopPropagation invocation...

Given the limited time to resolve that support ticket, we decided to use the react-onClickOutside instead of implementing our own HOC and let the components close themselves when handleClickOutside is triggered. After this change, e.stopPropagation() is only called in 7 files 5 times. Also #217 is addressed because ToolButton doesn't stop propagating events anymore and we just need to add a handler to the zoomOverlay element so that the overlay will close every time the item in it is clicked.

https://github.com/PDFTron/webviewer-ui/blob/ec7c989005f2ccc1f2a1c4b1b79626c4cdbafac4/src/components/ZoomOverlay/ZoomOverlay.js#L96-L121

Now back to this issue, I found that two steps are needed to resolve it. However I'm not sure if the code below will work for you because you mentioned that you are on 5.2, but as I stated above too many things happened in the 5.2 patch versions. If you are on 5.2.3 the code below should help.

1. handleClickOutside in the AnnotationPopup.js has a bug. It shouldn't close itself when we are clicking in the NotesPanel because NotesPanel has handlers to manage the opening/closing state for this component. So the function should be something like this:

   handleClickOutside = e => {
   const notesPanel = document.querySelector('[data-element="notesPanel"]');
   const clickedInNotesPanel = notesPanel?.contains(e.target);
   
   // the notes panel has mousedown handlers to handle the opening/closing states of this component
   // we don't want this handler to run when clicked in the notes panel otherwise the opening/closing states may mess up
   // for example: click on a note will call core.selectAnnotation which triggers the annotationSelected event
   // and opens this component. If we don't exclude the notes panel this handler will run and close it after
   if (!clickedInNotesPanel) {
   this.props.closeElement('annotationPopup');
   }
   };

2. After applying the above change I see that clicking on a note will open the AnnotationPopup but if I click the next note, it will still close. After some investigation I found it's caused by React batching the setState calls. Both core.deselectAllAnnotations(); and core.selectAnnotation(annotation); will trigger the annotationSelected event and AnnotationPopup listens to this event: https://github.com/PDFTron/webviewer-ui/blob/ec7c989005f2ccc1f2a1c4b1b79626c4cdbafac4/src/components/AnnotationPopup/AnnotationPopup.js#L115-L126

After core.deselectAllAnnotations(); in the Note is called, this.close(); in the AnnotationPopup will be called, which will close the component and call setState with the initialState. Then core.selectAnnotation(annotation); will trigger the invocation of

      if (annotations.length > 0) {
        const firstAnnotation = annotations[0];
        this.setState({
          firstAnnotation,
        });
      }

in the AnnotationPopup. This will call setState with firstAnnotation set. It seems that React will batch these two setState calls and as a result

https://github.com/PDFTron/webviewer-ui/blob/ec7c989005f2ccc1f2a1c4b1b79626c4cdbafac4/src/components/AnnotationPopup/AnnotationPopup.js#L58-L70

the code in the if block doesn't run, so the AnnotationPopup remains closed.

I can see a few options to fix this.

1. Use a setTimeout 0

   const handleNoteClick = e => {
   // stop bubbling up otherwise the note will be closed
   // due to annotation deselection
   e.stopPropagation();
   
   if (isSelected) {
   core.deselectAnnotation(annotation);
   } else {
   core.deselectAllAnnotations();
   setTimeout(() => {
     core.selectAnnotation(annotation);
     core.jumpToAnnotation(annotation);
   }, 0)
   }
   };

2. Move the code below to onAnnotationSelected with some modification: https://github.com/PDFTron/webviewer-ui/blob/ec7c989005f2ccc1f2a1c4b1b79626c4cdbafac4/src/components/AnnotationPopup/AnnotationPopup.js#L67-L70

3. "Hookify" AnnotationPopup because the setState returned by useState won't be batched.

The first option isn't ideal but I used it just for demo:

I will include the proper fix in the next release(6.0) so you can try it out after it's released. If this is urgent for you, could you try the options above and let me know how that works?

Also I think this reply covers #217, can you confirm if your questions in #217 are answered? If yes I will close #217.

[ZhijieZhang]

A known related issue and we plan to fix it in the future.

Any note clicking resulting in a document container scroll will close the AnnotationPopup. For example, If you were selecting a note in the first page and then click a note that's in the second page, AnnotationPopup won't show.

This one needs more effort to fix and due to the time constraints I will delay this a bit. But do please let me know if this issue is urgent.

[henry-chris]

Neither of these are urgent.

I actually reached similar conclusions to you when debugging, although you were much more versed in how to fix it.

I have some other things to do (I got pulled onto a different issue), but should be able to respond to #217 and this one by Monday at the latest.

Thanks for the quick update!

Are you guys planning to keep the react-onclickoutside package for good? i would want to move all of our custom components/modals/overlays to use that if so. I just started looking at it today, previously we just used our own HOC to handle stuff.

[ZhijieZhang]

Are you guys planning to keep the react-onclickoutside package for good? i would want to move all of our custom components/modals/overlays to use that if so. I just started looking at it today, previously we just used our own HOC to handle stuff.

Sorry for the delay in the response.

I can't say for sure but I feel like we may remove the use of it in the feature. One of the reasons is that we'd want to (re)write our components using hooks and I think there's a cleaner way to handle click outside. For example useOnClickOutside.

react-onclickoutside still requires the onClickOutside HOC for functional components that use hooks https://github.com/Pomax/react-onclickoutside#functional-component-with-usestate-hook, which I don't think is necessary...

Btw, I've rewritten AnnotationPopup.js using hooks in the dev branch and can confirm that the issue is fixed! The fix will be included in 6.0 and we are aiming to release it in the next 1-2 weeks.

[ZhijieZhang]

Hi Chris,

As I've replied in another issue, we've released a 6.0-beta, which should have fixed this issue. Please give it a try and let us know how it goes. Thanks!

[henry-chris]

Hi. It's going to be awhile (weeks at the least) as I have a lot of other things to get through before I merge or try another branch.

[ZhijieZhang]

Hey, thanks for letting me know.

I think I will close the ticket for now as I just verified the fix is in the 6.0-beta. Please feel free to reopen it if you found any issues. Thanks!