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

[BUG] Related items links do not open in new tab #5808

Open
2 tasks
cjcolvar opened this issue Apr 30, 2024 · 6 comments
Open
2 tasks

[BUG] Related items links do not open in new tab #5808

cjcolvar opened this issue Apr 30, 2024 · 6 comments
Assignees
Labels

Comments

@cjcolvar
Copy link
Member

cjcolvar commented Apr 30, 2024

Describe the bug
Related item links do not open in a new tab.

To Reproduce
Steps to reproduce the behavior, including the results:

  1. Go to https://avalon-dev.dlib.indiana.edu/media_objects/fj236208t
  2. Click on the Related Item link in the Details tab

Expected behavior
Related item links open in a new tab.

Release:
Occurs in 7.7+

Additional notes
This change in behavior comes in 7.7 since Ramp renders what's in the manifest and the manifest creates simple <a> elements without a target="_blank" attribute. This could either be fixed in the manifests Avalon generates and/or in Ramp by dynamically adding this attribute to certain links (metadata, homepage, etc.). It was decided that Ramp should be agnostic here and not open anything in a new tab unless explicit in HTML contained in the manifest.

See https://webaim.org/techniques/hypertext/hypertext_links#new_window.

Done Looks Like

  • For the Related Item field, Avalon manifests add the attribute for hyperlinks links to open in a new tab
  • Update manifest to include icon representing a link that opens in a new tab (in the a tag)
@joncameron
Copy link
Contributor

joncameron commented May 8, 2024

Discussion happened about whether this should live in the manifest or as a Ramp default; it was decided to put this into the manifest.

Generally we have an icon for when links open in a new tab. Can we add that in Ramp, and by what method?

IIIF Spec says: we could add an image tag to the hyperlink in the manifest, but this could also be in the Ramp.

@Dananji
Copy link
Contributor

Dananji commented May 8, 2024

After the discussion, I looked at other IIIF viewers to see how they handle linked metadata and saw the following behaviors in different viewers;

  • Mirador: opens all links in new tabs
  • Clover: opens rights in a new tab but not other links
  • UV: opens all links in a new tab

In all these cases the IIIF Manifest the metadata with links have not specified target="_blank" in the HTML for the <a> tag.

Even though my intuition was not to implement this on the Ramp side, since it's only suppose display what's in the IIIF manifest, and I was wrong?
The pattern others are following is the correct way to implement this?

@joncameron and @cjcolvar What are your thoughts?

@joncameron
Copy link
Contributor

joncameron commented May 10, 2024

Even though it's the behavior in those viewers I'm not convinced that opening links in new tabs/windows should be a default behavior in Ramp.

I read through some resources on this:

The general recommendation is to not break default behavior, and in almost all cases open links in the current tab or window. I feel comfortable sticking with that, and keeping the attribute to open a new tab just in Avalon's manifest generation rather than as something determined by Ramp.

An enhancement that might make this behavior better (active media playback is called out in some of the articles above as to why you might want to open links in a new context) is to retain more state in Ramp, so if a user clicks a hyperlink that opens in the current tab or window going Back in the browser returns to the media player to where it was, going back to the section and timepoint at which the user left it. I'm not sure if there's an issue for this but I can create one if not.

Another improvement that could be made here is adding "(opens in new tab)" text to the link label, as recommended in MDN and other resources.

@cjcolvar cjcolvar self-assigned this May 31, 2024
@cjcolvar
Copy link
Member Author

While working on implementing the solution to include target='_blank' in related item's <a> links I noticed that target isn't a supported attribute in the IIIF Presentation spec (https://iiif.io/api/presentation/3.0/#45-html-markup-in-property-values) so we were removing it during manifest generation. Even if we change Avalon to include the attribute ramp is ignoring it for the same reason. Should we change both of these? Or maybe we could add a prop in ramp to specify which metadata properties should open in a new tab then Avalon could just pass 'related_item'. This would require a lot of knowledge about the manifest and wouldn't be transferable to other players or even other Ramp instances.
@Dananji @elynema How should I proceed?

@Dananji
Copy link
Contributor

Dananji commented Jun 3, 2024

I looked at the other viewers to see how they handle this in the rights statement cookbook recipe:

  • both UV and Mirador inject target='_blank' to links in display
  • Annona: opens the links in the same tab, and doesn't link rights attribute, which is given as plain text in the Manifest without any HTML
  • Clover: adds target=_blank to rights attribute, but opens the other links in metadata in the same tab
  • Glycerine viewer: opens all the links in the same tab including rights attribute.

Looking at these, I think we could keep the behavior of any attributes that comes with any HTML in the Manifest as it is. i.e. if the value has any HTML tags in it, do not modify it except filtering any unsupported HTML attributes in it.

As for any values that are URLs presented as strings, I think we can handle them any way we like looking at what others are doing. But, for consistency as well and considering the resources Jon found, I think we can let them open in the same tab?

What do you think @cjcolvar ?

@cjcolvar
Copy link
Member Author

cjcolvar commented Jun 3, 2024

I think that is fine and would close this ticket 🙂 . If a user clicks a link that they were expecting to open in a new tab and it opens in same tab then going back to get back to the player is pretty easy. Doing this during playback may cause them to lose their place when they return to the player. If ramp were able to store/restore the playback state that would fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants