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

Bug in location hash processing loses any characters after second # #3811

Closed
Jermolene opened this issue Mar 2, 2019 · 20 comments
Closed

Bug in location hash processing loses any characters after second # #3811

Jermolene opened this issue Mar 2, 2019 · 20 comments

Comments

@Jermolene
Copy link
Member

@00SS discovered an interesting bug that has been in TW5 since 2014: the following code is used in boot.js to read the location hash (the portion of the current URL from the first # character):

$tw.utils.getLocationHash = function() {
	var parts = window.location.href.split('#');
	return "#" + (parts.length > 1 ? parts[1] : "");
};

The bug occurs when the location hash contains more than one # character. For example https://tiddlywiki.com/#Foo#Bar would be split into https://tiddlywiki.com/, Foo and Bar. The first string is thrown away and the second string is returned as the location hash, discarding the final string. This means that it is impossible to use a permalink to a tiddler that includes a # character. In practice, permalinks are supposed to escape # to %23 (and the permalinks generated by TiddlyWiki itself use this encoding), so I suspect that nobody has ever encountered this bug in the field.

It's easy enough to fix the bug, but @00SS also discovered an interesting side effect of the bug: navigating to a location hash that starts with a # character will not trigger TiddlyWiki's own navigation processing, but leave the underlying browser processing intact. This enables anchors to be used within tiddlers for sub-tiddler navigation.

So, rather than fix this bug, I propose to document the behaviour of double hash location hashes.

@Jermolene
Copy link
Member Author

@00SS would you be interested in making a PR for the required documentation? We'll need to both document the behaviour and how it can be used in practical terms, along the lines of your post.

@00SS
Copy link
Contributor

00SS commented Mar 2, 2019

@Jermolene
My pleasure!
☺️

@BurningTreeC
Copy link
Contributor

look, it's a bug!

no, itsa feature!

😁

👍

@Jermolene
Copy link
Member Author

Thanks @00SS

@00SS
Copy link
Contributor

00SS commented Mar 3, 2019

Just as an interesting note, if the underlying HTML is inspected, this code produces the same HTML as using the <a href ... </a> method :

<$set name="tv-get-export-link" value="##s004">
<$link>Test link to id=#s004</$link>
</$set>

gives: <a class="tc-tiddlylink tc-tiddlylink-resolves" href="##s004">Test link to id=#s004</a>
The difference is that this has a click event associated with it:

function(event) {
  eventInfo.handlerObject[eventInfo.handlerMethod].call(eventInfo.handlerObject, event);
}

The documentation though clearly points out:

This variable has no useful effect when TiddlyWiki is running in a browser, as the href attribute is ignored there – links between tiddlers are performed by JavaScript instead. The variable comes into play when one is using the Node.js configuration to generate a static version of a wiki.

@00SS
Copy link
Contributor

00SS commented Mar 5, 2019

I am working on the documentation and put in some posts on the same Google Groups discussion: Section Names using ID for internal Links

I wanted to see what feedback is received before making the pull request for documenting this. For the record, I am attaching the 2nd draft (below) - which can also for now be seen here: Internal Links using HTML

Attached tiddler: Internal Links using HTML - updated 1.tid.txt

00SS added a commit to 00SS/TiddlyWiki5 that referenced this issue Mar 7, 2019
A new tiddler documenting the process for creating anchors and using links for sub-tiddler navigation.
Arising from issue TiddlyWiki#3811 & Discussed on Google Groups: [Section Names using ID for internal Links](https://groups.google.com/forum/?hl=en#!topic/tiddlywiki/ikjxwjllFtE)
@twMat
Copy link
Contributor

twMat commented Mar 9, 2019

Is it possible to modify the current [[link]] mechanism to take embrace this discovery? My point is that TW users should not be forced to learn or mix in html syntax (and quirks) so the ideal would be a [[link]] mechanism that can target both tiddler titles and these "internal links" (...perhaps via syntax [[tiddlertitle##anchor]] or similar)?

@00SS
Copy link
Contributor

00SS commented Mar 9, 2019

I also thought about this, but since the [[link]] mechanism relies on the $link widget, it all depends on that being made to work. Another syntax may be [id:##anchor[Target Tiddler]] or just [##anchor[Target Tiddler]]

@Jermolene
Copy link
Member Author

With respect to @00SS's comment above about the tv-get-export-link variable is being respected despite the documentation, note that the docs say "no useful effect", not that it doesn't work. I guess you've found a use for it :)

@twMat I don't see a way we can easily reuse the <$link> widget to make links to anchors.

@twMat
Copy link
Contributor

twMat commented Mar 11, 2019

For the record, so my main point is not missed :

There is a problem to expect/instruct TW users to have to learn html for such a basic thing as creating links. Of course, html is better than no solution at all but my hope is that this discovery should be "tiddlified" and not force users to go outside of wikitext.

I feel I'm stating the obvious here had it not been for the fact that fellow @00SS is putting a lot of great work into documenting this as a html solution. Or, @Jermolene , is it not possible to "embed" this discovery into wikitext at all?

@00SS
Copy link
Contributor

00SS commented Mar 11, 2019

I agree with you @twMat
Linking is used so much at even the most basic use level that it needs to be easily written in a WikiText format.

There are still some as yet unexplained issues that come up - such as some browsers not accepting URI encoded id attributes as anchors. And also, on and off, (I estimate 1 out of 4 times) when I have made changes to an id or link in the DOC tiddler or reload TiddlyWiki after changes, the first click on an anchor link just goes to the top of the Story, but after that, every anchor link click works perfectly.

So maybe before a WikiText solution is designed, we can put up the HTML way of doing it for some months and see what problems and requirements arise so that we can have a robust WikiText solution.

@00SS
Copy link
Contributor

00SS commented Mar 12, 2019

I think I will put a stronger discouragement message for an id with spaces just in case the scrollIntoView() javascript idea from #2271 is used at some later time.

@00SS
Copy link
Contributor

00SS commented Mar 12, 2019

To make a note of previous discussions and ideas:

Reading this Github post in issue #1783 led me to: scroller.tiddlyspot.com which uses scrollIntoView() in a modified $:/core/modules/widgets/link.js

In that custom modified TiddlyWiki, in a tiddler TiddlerTitle, defining a !A Heading auto creates an <h1 id="TiddlerTitle_-_A_Heading">A Heading</h1>

The WikiText to open the closed tiddler and scroll to the Heading is: [#[TiddlerTitle - A Heading#Pretty Name|TiddlerTitle]]

I believe also that scroll to align the top/left of the target element with the top/left of the viewport is another piece of the puzzle in place.

A way to stop the URL in the bar being updated might be found here: intlinks.tiddlyspot.com

@twMat also mentions another a new possibility on Issue #1783

@00SS
Copy link
Contributor

00SS commented Mar 24, 2019

Apologies for the delay from my side. I hope to get around to completing this soon.

@rmunn
Copy link
Contributor

rmunn commented Oct 30, 2020

@Jermolene wrote:

So, rather than fix this bug, I propose to document the behaviour of double hash location hashes.

While the side effect (allowing double-hash values to be treated specially) is interesting and probably worth preserving, I think there's also a strong use case for fixing this bug, because there will definitely be people who write tiddlers with titles like "Meeting #1 on October 20", which could then trigger the bug if the tiddler title somehow failed to get URL-escaped.

Note that it would not work to simply do .split(',', 2) because Javascript's .split() method throws away any text after the limit. So by setting a limit of 2, you'd get the parts before and after the first # but not the second # or anything beyond it. (This is different from the behavior of most other languages with a string-split feature, where 'Foo#Bar#Baz' split on # with a limit of 2 would produce ['Foo', 'Bar#Baz'].)

But this bug can be solved very easily by using .indexOf('#') instead of .split(), and that would allow us to preserve the behavior quite easily. Take the index of the first #, and first check if there is another # at (index+1), of course making sure you don't try to access beyond the length of the string. If there's another # at (index+1), then that was two # in a row, so follow the original bug's side effect of returning a single # for the location hash. If the character at (index+1) is anything other than a #, then just return the substring from (index+1) to the end of the original string. Done: we've fixed the bug for tiddler titles that contain a # somewhere in the middle, but we've preserved the side-effect of letting ##foo trigger special behavior. And since that's now explicitly checked for in code rather than a side effect that looks unintended, we've greatly reduced the chance that some contributor will later come along, spot the bug, and fix it without finding this issue. So once that's done, it should be safe to document the special ##foo behavior.

@rmunn
Copy link
Contributor

rmunn commented Oct 30, 2020

@Jermolene PR #4947 would fix the bug for tiddler titles like "Meeting #1 on October 20", while still preserving the desired behavior of allowing double hashes like ##foo to skip TiddlyWiki's navigation behavior. I haven't written anything to document that behavior, as I don't fully understand its implications yet. I understand them a little, I think, but not well enough to explain them. So I'll leave documenting the use of double hashes to someone else.

@rmunn
Copy link
Contributor

rmunn commented Oct 30, 2020

Looks like #3836 was a good PR for documenting this behavior. It was ultimately closed, but I think I'll revive it at some point. In the Google Group there was also another piece of documentation contributed by Watt, which I also think is worth preserving in case it's better for the PR. I'm going to attach both of those documents to this issue until I can find the time to create a PR.

Both of those look good at first glance, though whoever does this PR (whether that's me or someone else) will have to read through them and make sure they are still up-to-date for 5.1.23, or update them if any recently-added features mean the docs need a change or two.

(Note to self: #3767 and #3926 are also worth looking at when writing the documentation PR).

@Jermolene
Copy link
Member Author

Thanks @rmunn I think this is a reasonable fix

@rmunn
Copy link
Contributor

rmunn commented Nov 30, 2020

#5149 is a PR for reviving #3836. Once it's merged, this bug can safely be closed, I think.

@Jermolene
Copy link
Member Author

Thanks @rmunn

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

No branches or pull requests

5 participants