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

fix(tXml): html unescape node attributes with urls #6267

Merged
merged 4 commits into from Feb 23, 2024

Conversation

dave-nicholas
Copy link
Contributor

To provide feature parity with the DOMParser.getAttribute API which would unescape html entities for free this PR unescapes url strings which come from node attributes.

In a similar vain to #6198 which unescapes child strings.

@joeyparrish
Copy link
Member

The other PR involved a smaller change, in which the parsing change was inside Txml instead of at every call site. I think I would prefer this to go into Txml, too, unless there is some good reason to do otherwise.

@shaka-bot
Copy link
Collaborator

Incremental code coverage: 100.00%

@dave-nicholas
Copy link
Contributor Author

The reason not to do that in this case is to save calling the unescape function whilst parsing every attribute when it is only needed in a few places. The change in the other PR was possible because I had already made a change at every callsite from element.textContent to TXml.getContents(element) in the initial PR. I thought about making a similar method in the TXml library but that method would just call StringUtils.htmlUnescape anyway.

@absidue
Copy link

absidue commented Feb 22, 2024

As this custom XML parser is exposed to API users via the dash manifest preprocessor option, things that happened automatically before but don't now, should probably be documented somewhere. As that will make it easier for developers to migrate their projects to the new XML parser.

@dave-nicholas
Copy link
Contributor Author

@absidue that is a fair point and I have updated the upgrade documentation to reflect this.

docs/tutorials/upgrade.md Outdated Show resolved Hide resolved
docs/tutorials/upgrade.md Outdated Show resolved Hide resolved
@joeyparrish
Copy link
Member

You've convinced me on your approach. If we can agree on the docs, I'll be happy to merge it. Thanks for your contribution!

@joeyparrish joeyparrish changed the title feat: html unescape node attributes with urls fix(tXml): html unescape node attributes with urls Feb 23, 2024
@joeyparrish joeyparrish merged commit 67cd2dd into shaka-project:main Feb 23, 2024
16 checks passed
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 23, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Apr 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants