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

Add target to attribute whitelist, closes #29 #30

Closed
wants to merge 1 commit into from

Conversation

diegosalvi
Copy link
Contributor

No description provided.

@@ -162,7 +162,7 @@ private static void forceReleaseBuffer(ByteBuffer buffer) {

public static String cleanPageText(final String dirtyPageText) {
final Whitelist whiteList = Whitelist.relaxed();
whiteList.addAttributes(":all", "class", "id", "href", "docetref", "title", "package", "src");
whiteList.addAttributes(":all", "class", "id", "href", "docetref", "title", "package", "src", "target");
Copy link
Contributor

@mcRoot mcRoot Sep 19, 2017

Choose a reason for hiding this comment

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

Agree with the change: can't think of any disadvantage but, maybe would be better to double-check that target attribute is adopted only on anchor referring to external target so as to avoid unexpected side-effects when working with internal links. This can be done by acting on validation checks on Docet maven Plugin for instance and, anyway, making sure, as well, that no target is present on an internal targeted anchor at runtime: the latter can be done by adding also runtime checks on DocetManager.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mcRoot see the discussion on #29
If the documentation writer uses the target attribute and Docet leaves it as it is the behaviour is unpredictable.
We could allow such attribute as you say at most in the case that the href is to an external target.
IMHO this is very important, Docet-ML is not a generic HT-ML !

@diegosalvi diegosalvi closed this Sep 25, 2017
@diegosalvi diegosalvi deleted the AnchorTarget branch September 25, 2017 08:17
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.

None yet

3 participants