Skip to content

Conversation

cybrox
Copy link
Contributor

@cybrox cybrox commented Jul 14, 2017

When merged, this will fix #509

Firefox (and probably other browsers) expect the URI of an anchor to be urlencoded
but the actual target to be NOT encoded. Due to this behaviour, links to functions
with special characters won't work.

While RFC3986 is very clear on how URIs have to be formatted, there is no clear
specification on how link targets actually should (or shouldn't) be encoded.
There are lose guidelines for HTML4/HTML5 but they rarely cover special characters.

Since most browsers seem to have adapted to also match encoded anchor targets,
this commit does not change but only extend the current behaviour by adding a
second, unencoded target to the template, similar to what is already done when
linking to a function with optional parameters, which results in target links
with different arity.

@sourcelevel-bot
Copy link

Hello, @cybrox! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@josevalim
Copy link
Member

Won't this generate duplicate IDs for most cases though? And I am assuming that if we drop the enc_ in the line above is going to break the behaviour in some browsers, right? Maybe we should test that though?

@cybrox
Copy link
Contributor Author

cybrox commented Jul 14, 2017

@josevalim You are right, no idea how I didn't realise that. While it would technically work, it's definitely not a clean solution according to the specs.

I have not yet found a browser that caused problems with the non-encoded id's using only h but there definitely is a chance, that we just invert the problem. So far, I successfully tested it on:

  • Win7 - Internet Explorer 11, Firefox 54.0, Chrome 61.0
  • OSX10.12.5 - Chrome 59.0, Firefox 54.0

The more pragmatic - and probably safer - solution in terms of browser support would probably be to just wrap the unencoded anchor in an if with a helper that determines if the encoded string is different from the raw, escaped string. Reasonable?

@josevalim
Copy link
Member

josevalim commented Jul 14, 2017

@cybrox the if approach sounds good to me, thanks!

@cybrox
Copy link
Contributor Author

cybrox commented Jul 14, 2017

Re-pushed the feature branch with the respective helper :)

|> h()
end

@doc false
Copy link
Member

Choose a reason for hiding this comment

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

I think this function can be private because the template is executed in this module? Or have you ran into issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works fine when private and I don't think a similar issue will arise with epubs, I'll make it private.

@josevalim josevalim merged commit 4c45bc0 into elixir-lang:master Jul 14, 2017
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@cybrox cybrox deleted the fix-ff-anchor branch July 14, 2017 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Anchor for <<>> is not working in Firefox
2 participants