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

New: Flag to control xlink prefix on href #2080

Merged
merged 3 commits into from
Feb 26, 2021
Merged

New: Flag to control xlink prefix on href #2080

merged 3 commits into from
Feb 26, 2021

Conversation

ahankinson
Copy link
Contributor

After some extensive testing it seems that some browsers, in some cases, will not display SVG if the href attribute is prefixed with xlink, a change that was introduced with SVG 2.0. This is not yet a recognized specification, but the changes in browsers are being enacted nevertheless.

On the other hand, some strict SVG 1.1 clients will not render the SVG correctly if it doesn't include the xlink: prefix. In my testing this included Inkscape.

Given that the most likely target for Verovio output is browsers, but still wishing to retain backwards compatibility, this PR changes the default behaviour from producing xlink:href to simply href. Clients that need the full attribute name can set --svg-include-xlink which will insert the xlink prefix on the href attributes.

References previous discussions on this topic: #1158, #332

After some extensive testing it seems that some browsers, in some cases, will not display SVG if the `href` attribute is prefixed with `xlink`, a change that was introduced with SVG 2.0. This is not yet a recognized specification, but the changes in browsers are being enacted nevertheless.

On the other hand, some strict SVG 1.1 clients will not render the SVG correctly if it *doesn't* include the `xlink:` prefix. In my testing this included Inkscape.

Given that the most likely target for Verovio output is browsers, but still wishing to retain backwards compatibility, this PR changes the default behaviour from producing `xlink:href` to simply `href`. Clients that need the full attribute name can set `--svg-include-xlink` which will insert the xlink prefix on the href attributes.

References previous discussions on this topic: #1158, #332
@ahankinson ahankinson requested a review from lpugin February 26, 2021 12:06
Copy link
Contributor

@lpugin lpugin left a comment

Choose a reason for hiding this comment

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

Have you tested with the PDFKit js toolkit used in MEI Viewer?

src/toolkit.cpp Outdated Show resolved Hide resolved
@ahankinson
Copy link
Contributor Author

ahankinson commented Feb 26, 2021

I've tested it and it seems to work as expected; including or removing xlink: doesn't seem to make a difference with pdfkit. I'm attaching a copy of a quick test file I put together; the verovio-toolkit.js was built with the source from this PR, but the other libraries are the same as those used with the viewer.

Easiest way to test it is to cd into this directory and then run a quick Python HTTP server: python3 -m http.server 8007, then visit http://localhost:8007/test.html in your browser. It should start to download a PDF file.

You can control the xlink behaviour by commenting / uncommenting the appropriate option in the test.html source. (It should be obvious).

test-pdfkit.zip

@rettinghaus
Copy link
Contributor

Does it has to be independent from option svgHtml5? Or could those functionalities be combined in a svg2 option?

@ahankinson
Copy link
Contributor Author

Laurent and I had a quick discussion about this, and decided that, since 1) SVG2 isn't official yet, but 2) browsers have started to implement it in piecemeal, that it's better to address the specific problem directly, rather than try to do a full SVG2 update. (Not to mention that there's not really a SVG 2 validator out there yet...)

@ahankinson
Copy link
Contributor Author

ahankinson commented Feb 26, 2021

For reference, the full issue report is here: elm/svg#31

The issue is that browsers only support href on SVG that is attached to the shadow DOM; it's not a problem (either way) if you are doing innerHTML and appending it directly to the DOM, but if you're using a JavaScript toolkit that does it's rendering calculations outside of the DOM and then doing diff updates, the ID references are not resolved, meaning the <use> element does not find the element ID it's supposed to be pointing to.

To retain support for SVG 1.1 by default, the xlink flag is now used to control whether the namespace prefix is removed, rather than added.
@ahankinson
Copy link
Contributor Author

ahankinson commented Feb 26, 2021

OK, after talking it over a bit more with @lpugin we decided to flip the behaviour of the flag; the old behaviour is now the default, and the new behaviour is enabled with a flag.

std::string hrefAttrib = "href";
if (m_includeXlink) {
if (!m_removeXlink) {
Copy link
Contributor Author

@ahankinson ahankinson Feb 26, 2021

Choose a reason for hiding this comment

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

FYI, I did it this way to avoid needing to strip characters off the string, and instead prepending to the string by default. This avoids needing to make a new copy of the string, or doing any index-based counting to remove the prefix.

@lpugin lpugin merged commit 37d85f1 into develop Feb 26, 2021
@lpugin lpugin deleted the fix-xlink-on-href branch February 26, 2021 15:45
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