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

404 page: navigation links are broken #1598

Closed
arnaudbreton opened this issue Aug 21, 2018 · 11 comments
Closed

404 page: navigation links are broken #1598

arnaudbreton opened this issue Aug 21, 2018 · 11 comments
Labels

Comments

@arnaudbreton
Copy link

@arnaudbreton arnaudbreton commented Aug 21, 2018

Issue

When hitting a missing page (404), the navigation links are not working anymore.
The page link contains the base URL.

Steps to reproduce

  1. Visit https://arnaudbreton.github.io/mkdocs-404-nav-issue/hello => works. HW nav link is well formed.
  2. Go to https://arnaudbreton.github.io/mkdocs-404-nav-issue/hello/missing.
  3. Hover the "HW" nav link. It's "/hello/hello/" when it should "/hello".
  4. Clicking on it keep appending the previous path.

Source repository is here: https://github.com/arnaudbreton/mkdocs-404-nav-issue
It's as minimal as possible to nail down the issue.

I hope I don't do things the right way.
Thanks for your help!

@waylan
Copy link
Member

@waylan waylan commented Aug 21, 2018

I notice you do not have the site_url config setting set. I suspect the issue would be resolved by setting that setting. See this comment for a detailed explanation of why that would matter.

That said, we should be able to make a guess if/when the site_url is not set. I think a reasonable approach would be that if it is not set, assume the mkdocs root is at the server root. In other words, for error pages, base_url should be / if site_url is None.

This problem cannot be reproduced with the dev sever as the dev server overrides the site_url setting to have it point at the dev server. Therefore, when using the dev server, the site_url setting is always set. However, we can't make the site_url "required" as users who build their docs for browsing on the file system (file://) do not need or want the site_url setting.

And that is why having a default seems to be the best approach. For those who host their sites in subdirs, they simply need to set the optional setting (site_url). For everyone else, it "just works".

Loading

@arnaudbreton
Copy link
Author

@arnaudbreton arnaudbreton commented Aug 21, 2018

@waylan thx for the quick feedback!

This problem cannot be reproduced with the dev sever as the dev server overrides the site_url setting to have it point at the dev server

You mean mkdocs serve, right? If yes, I can reproduce there too.

Loading

@arnaudbreton
Copy link
Author

@arnaudbreton arnaudbreton commented Aug 21, 2018

Also, on our production doc - the one I'm troubleshooting - we've set the site_url to our doc root:

...
site_url: https://docs.sqreen.io/
...

As you can see visiting https://docs.sqreen.io/iam/missing, it breaks the nav too.

Loading

@waylan
Copy link
Member

@waylan waylan commented Aug 21, 2018

Well I just found a bug. If base_url is / it is getting stripped to an empty string. Turns out we had a couple bad tests. And as this doesn't apply in the dev server, we missed it. Unfortunately, for 1.0.0 and 1.0.1, setting the site_url to the server root is not a workaround. We'll try to get a fix released ASAP.

Loading

@waylan
Copy link
Member

@waylan waylan commented Aug 21, 2018

I just realized why the problem exists. Previously, we did not have the url template filter which uses base_url to properly adjust URLs. Therefore in templates one would do {{ base_url }}/path/to/file. Note that if base_url was /, then the generated URL would be //path/to/file which is obviously wrong. Therefore, we correctly stripped base_url to an empty string in that case. Of course, if base_url was /foo/, striping the ending slash was needed.

Now that we have the url filter, paths are passed to the filter, which is smart enough to do the correct thing. The problem is that, at a minimum, the filter needs base_url to be / for error pages. otherwise it will output a relative URL. And for previously explained reasons, error pages cannot have relative URLs.

The fix in #1599 resolves this, expect when templates still use the old manual concatenation method for building URLs. If a template author really wants to use manual concatenation, they may need to jump through a few hoops (if statements) for it to work.

Loading

@arnaudbreton
Copy link
Author

@arnaudbreton arnaudbreton commented Aug 21, 2018

Thanks a lot @waylan for the very comprehensive answers. Glad it helped nailing down this issue.

Let me know if you want us to test before the real, if it helps.

Loading

@waylan
Copy link
Member

@waylan waylan commented Aug 21, 2018

Sigh, this also breaks search. We are concatenating URLs at search/main.js#L13, search/main.js#L70, and search/main.js#L81. There is the URL() constructor, but that is not supported across all browsers (IE being the problem). Alas, there is a polyfill for it. See also URI.js, which also lists various other alternatives.

Loading

@waylan
Copy link
Member

@waylan waylan commented Aug 21, 2018

Well the URL constructor won't work. From the Chrome console:

> new URL('foo/bar', '/')
VM151:1 Uncaught TypeError: Failed to construct 'URL': Invalid base URL
    at <anonymous>:1:1
> new URL('foo/bar', 'http://example.com/')
URL {href: "http://example.com/foo/bar", origin: "http://example.com", protocol: "http:", username: "", password: "", …}

The base URL needs to be a full protocol/domain. For our use, its probably easier to just write a simple function. It's only ever needed in the one file anyway.

Loading

@waylan waylan closed this in #1599 Aug 21, 2018
waylan added a commit that referenced this issue Aug 21, 2018
@barreeeiroo
Copy link

@barreeeiroo barreeeiroo commented Aug 21, 2018

First of all, thanks for fixing the issue @waylan
And then, could be possible to know an estimated release date for 1.0.2? As I'm using mkdocs at docs.makeroid.io, I'm facing the 404 issue, and I would like to know when it could be fixed
Thanks in advance

Sent from my MI6 using FastHub

Loading

@waylan
Copy link
Member

@waylan waylan commented Aug 22, 2018

When I have the time. Hopefully tomorrow, but we'll see.

Loading

@arnaudbreton
Copy link
Author

@arnaudbreton arnaudbreton commented Aug 31, 2018

I can confirm it works like a charm now. Thx a lot!

Loading

robzor92 added a commit to logicalclocks/logicalclocks.github.io that referenced this issue Oct 1, 2021
moritzmeister pushed a commit to logicalclocks/logicalclocks.github.io that referenced this issue Oct 1, 2021
moritzmeister added a commit to logicalclocks/logicalclocks.github.io that referenced this issue Oct 1, 2021
robzor92 pushed a commit to logicalclocks/logicalclocks.github.io that referenced this issue Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants