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

MBS-11040: Use expand2react for annotation display #1653

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reosarevok
Copy link
Member

Implement MBS-11040

The html tags legitimately generated by annotations are added to htmlTagName (should already have been added really, since the same are in use for collection descriptions).

expand2react does struggle with malformed returns from Text::WikiFormat more than dangerouslySetInnerHTML, which ignores plenty of the issues, but that should be fixed at the source, I expect.

We can choose to merge this as-is, or we can choose to try and finally deal with the Text::WikiFormat mess. If we opt for the latter, we can:

  • Improve the Text::WikiFormat module
  • Try and find a JS library that does the work for us
  • Write our own code to do this (which is probably not super hard, famous last words and all that)

The html tags legitimately generated by annotations are added to
htmlTagName (should already have been added really, since the same
are in use for collection descriptions).

expand2react does struggle with malformed returns from Text::WikiFormat
more than dangerouslySetInnerHTML, which ignores plenty of
the issues, but that should be fixed at the source, I expect.
@reosarevok reosarevok added the Refactoring Refactoring-only PRs (eslint fixes etc) label Aug 17, 2020
@reosarevok reosarevok requested review from mwiencek and yvanzo August 17, 2020 12:50
@mwiencek
Copy link
Member

This seems like a good idea in general, but I'm also afraid that if expand2react throws an exception (because it couldn't parse the HTML), it'll crash the entire page and just return a cryptic JS stack trace to the user. So I feel like we should find a way to deal with Text::WikiFormat first. (I did have a branch working on a replacement, but it's not a priority right now.) Otherwise we may have to wrap all the expand2reacts in try/catch and fall back to dangerouslySetInnerHTML. Another option might be to add a loose/non-strict mode to expand2react that silently ignores broken HTML. Not sure.

@yvanzo
Copy link
Contributor

yvanzo commented Sep 2, 2020

Otherwise we may have to wrap all the expand2reacts in try/catch and fall back to dangerouslySetInnerHTML.

I would favor this option because 1) dealing with Text::WikiFormat is way out of current priorities and will eventually be dropped out and 2) having a loose mode to expand2react might open security breaches elsewhere or in the future.

@mwiencek
Copy link
Member

mwiencek commented Sep 2, 2020

I was thinking the loose mode would still only be able to output the same restricted elements as currently, just would ignore any tags that are broken or mismatched - they'd be HTML encoded, of course, since React doesn't allow otherwise without dangerouslySetInnerHTML. So it'd still be restricted (whereas dangerouslySetInnerHTML wouldn't be restricted at all).

@reosarevok reosarevok marked this pull request as draft December 9, 2021 18:29
@reosarevok
Copy link
Member Author

We need to figure out what we actually want to do here, so setting to draft for now.

@reosarevok
Copy link
Member Author

Actually, I'm half-tempted to just close this for now - the reports code has changed for example and now annotations just use a default defineTextHtmlColumn. Another option would be to just have that use the same loose validation proposed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Refactoring-only PRs (eslint fixes etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants