#1554 Add space to empty anchor tags to fix internal links.

[rpedela]

Internal links (anchor tags) are not linked correctly when the anchor's content is empty. This adds a space with a height of 0 when it is empty so that internal links are linked correctly.

@partychen deserves credit for this fix. I am just making the PR.

[ashkulz]

This should be outside i.e.

QString h=elm.attribute("href");
if (h.isEmpty()) h=elm.attribute("ns0:href");

// Internal links ...
if (elm.firstChild().isNull()) {

Otherwise, this will get executed only when there is no href attribute but only a ns0:href attribute -- unless that is what is wanted?

[rpedela]

The logic is that if the a tag has an empty href attribute then it must be an anchor tag, but it only adds the space if there aren't any children. I tested the following scenarios which all worked:

1. <a name="abc"></a>
2. <a id="abc"></a>
3. <a name="abc" href=""></a>
4. <a id="abc" href=""></a>

If I move the code outside then it will execute for all a tags and will replace the link text with a space. I tested that just to make sure.

[ashkulz]

Okay, so then this approach is wrong/incomplete. The ns0:href is generated by the TOC support, so it is quite possible that it won't work there. A better approach is to check directly for the condition that you want (<a name="not_empty"></a>) rather than the absence of something (missing href on <a>) and work on that directly. Additionally, you might want to check for id tags on headers/divs and add some content there too (they can also be used as the target of an href).

[rpedela]

The check could be changed to also check that the a tag has an id or name too. That would be a stronger check and less likely to produce side effects. However an empty href attribute is the key condition for an a tag.

For ns0:href, are you saying that we also need to check that ns0:href is empty?

[ashkulz]

Yes, the TOC is generated via XSLT which unfortunately adds tags with the ns0: namespace, which is why you see code for that being specifically handled.

[rpedela]

You make a good point about other tags being used as targets which I didn't think about. Here are the cases I can think of:

1. An a tag with either id or name attributes, empty href or ns0:href attributes, and no children.
2. Any other anchor tags (e.g. div, p, h1) that have an id and no children. I can look up the full list of tags than can be used as an anchor.

Am I missing any cases?

[ashkulz]

Looks OK to me. Also, just noticed -- please target master and not the 0.12.3.x branch.

[ashkulz]

Also, please rework the commit message (see previous commits) so that it includes the comments you have put in the PR as a part of the commit message. Additionally, I'd prefer to keep the comments out of the code (git blame will tell you why the change was made).

You can use `git commit --author "Name Email@Domain" to override the author, with you as the committer. Please make sure you rebase/amend the commit so that there is only a single commit which needs to be merged.

[ashkulz]

@rpedela: any update?

[rpedela]

Nope. I haven't had time to work on it. Hopefully soon.

[rpedela]

I have tested <div id="something"></div> and <div name="something"></div> which worked without any code changes, and based on my understanding of the code it should work for any other HTML tag with an id. I think it is just <a id="something"></a> and <a name="something"></a> which are broken.

Also, just noticed -- please target master and not the 0.12.3.x branch.

How about both master and 0.12.3.x? This is pretty major bug in my opinion, and I would prefer to not use master in production. Of course, I could use my own fork until the next stable release, but I am sure I am not the only one who needs this bug fixed in production.

[ashkulz]

I'm planning to release 0.12.4 pretty soon, so it will be in the next stable release. I keep 0.12.x.x only for critical security updates in bundled libraries e.g. OpenSSL.