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

Islandora 2060 #82

Merged
merged 2 commits into from
Sep 13, 2017
Merged

Islandora 2060 #82

merged 2 commits into from
Sep 13, 2017

Conversation

patdunlavey
Copy link

@patdunlavey patdunlavey commented Sep 11, 2017

JIRA Ticket: ISLANDORA-2060

What does this Pull Request do?

The problem

When content is loaded via an ajax call on a page that has an Openseadragon viewer, the viewer replicates itself inside the #islandora-openseadragon div.

The Drupal.behaviors.islandoraOpenSeadragon:detach function is being called during the ajax call, and this function is disabling but not removing the viewer. It also removes Drupal.IslandoraOpenSeadragonViewer[base].

After the detach event is done, a new attach event is triggered. When the attach function is called, it sees that there is no Drupal.IslandoraOpenSeadragonViewer[base], so it adds a new one, thus duplicating the viewer.

Desired outcome

Ajax calls within a page displaying an openseadragon viewer should not cause the viewer to be duplicated.

What's new?

Adds a condition to the Drupal.behaviors.IslandoraOpenSeadragon.detach function which checks to see if the context of the detach event is the openseadragon viewer. If so, it goes ahead with the detach. But if not, then it does not detach.

How should this be tested?

  • To reproduce the duplication problem, on the page displaying the openseadragon viewer you would need to have a link that makes an ajax call.
  • Once that is implemented, this PR should cause the openseadragon viewer to not be duplicated.

Additional Notes:

  • The detach function would need to function in the case where a module wishes to create and delete openseadragon viewers. For example, if clicking on a search results pops the viewer up in a colorbox, then the close function for that may need to detach the viewer. This has not been tested!

Interested parties

Tag @NigelBanks, @Islandora @7-x-1-x-committers

Pat Dunlavey added 2 commits September 8, 2017 15:21
…wer upon ajax call

Why:

* When a page containing an Openseadragon viewer also has a link that triggers an ajax call, clicking on that link causes the viewer to be duplicated.
* The cause appears to be that an ajax call call triggers detach and attach functions for all Drupal.behaviors.
* islandora_openseadragon’s detach function causes the old viewer to be disabled, but not removed or hidden, and then the subsequent attach function creates a new viewer.
* I don’t understand the current logic of the attach and detach functions, but I assume they serve a purpose!

This change addresses the need by:

* Because I don’t understand the use case that the detach function was designed for, I chose to add a condition within that function which tests to see if the DOM element that was clicked is inside the Openseadragon viewer. Only then does the normal detach behavior run.
* This prevents the detach function from disabling the viewer when the DOM element that triggered the ajax callback is elsewhere on the page.
* I don’t know if this is the best solution, or even an acceptable one!

Reference Links:

* https://jira.duraspace.org/browse/ISLANDORA-2060
@DiegoPino
Copy link

@patdunlavey @nigelgbanks this one is an improvement and we are close to code freeze. Please give this a push/search for reviewers in the committers group if you want to get this into code hopefully before the 15th! Good luck =)

Copy link
Member

@nigelgbanks nigelgbanks left a comment

Choose a reason for hiding this comment

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

This looks good to me, under default Islandora setup the detach function doesn't normally get called so no change in behaviour for 99% of users. In the case of users who have need to perform some Ajax calls on object pages that display the openseadragon viewer this improvement will prevent the issue described.

@nigelgbanks
Copy link
Member

I'll merge after the standard 24 hours (assuming travis passes etc), unless someone else wants to chime in.

@DiegoPino
Copy link

That was quick! Travis is still runnig so make sure that passes first. Thanks to both, great response!

@patdunlavey
Copy link
Author

Thanks for giving this priority, @nigelgbanks !

@nigelgbanks nigelgbanks merged commit cdb1aa5 into Islandora:7.x Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants