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

Fix navbar click while in a search #45812

Merged
merged 3 commits into from Nov 11, 2017

Conversation

GuillaumeGomez
Copy link
Member

Fixes #45790.

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @frewsxcv

(rust_highfive has picked a reviewer for you, use r? to override)

@QuietMisdreavus
Copy link
Member

This does not fix the issue. The URL still swallows the #method.something anchor, meaning that the link still doesn't work. It's just that now the search box goes away when you click it, leading you to think that maybe something's happening, when in fact it's still busted.

@GuillaumeGomez GuillaumeGomez force-pushed the links-and-search branch 4 times, most recently from 57e0d15 to 3533b2a Compare November 6, 2017 21:39
@QuietMisdreavus
Copy link
Member

So with the current change, the search properly works, and clicking a link in the sidebar will properly dismiss the search, scroll to the desired heading, and toggle the heading's "active" highlight (giving it the yellow background). However, something strange happens: The window title is not changed back from "Results for (query)", and the URL will only say ?search=query, and the #method.into_inner hash will be absent from the URL. After a discussion in IRC we're not sure what's exactly going on here, but i'm not comfortable accepting this as-is with that behavior.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2017
@frewsxcv
Copy link
Member

frewsxcv commented Nov 7, 2017

not too familiar with this code, so gonna reassign

r? @QuietMisdreavus

@GuillaumeGomez
Copy link
Member Author

Updated.

@QuietMisdreavus
Copy link
Member

Is it possible to clear out the query parameters when setting the hash? The URL now properly has the hash, but it still has the query parameters, so now it has ?search=sync#method.into_inner. If someone accesses a link with both of these items, the query parameters take over; the hash is discarded and the search loads. For example, try this link in a rendering of this PR.

Also the title doesn't get set back when you click a sidebar link, so it still says "Results for sync" even though the search results were dismissed. >_>

removeClass(document.getElementById("main"), "hidden");
var hash = ev.newURL.slice(ev.newURL.indexOf('#') + 1);
if (browserSupportsHistoryApi()) {
history.replaceState(hash, "std - Rust", "?search=#" + hash);
Copy link
Member

Choose a reason for hiding this comment

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

  • Firefox ignores the title argument to pushState/replaceState anyway, so right now this is moot, but:
  • Setting this unilaterally to "std - Rust" is wildly inaccurate, even within libstd, because for anything but the crate root the title is set to the path of the item. But for anything but libstd, this is just erroneous.

On the other hand, with the empty search param, the link is now stable, and the search box and results are still the same. If we can't change the title (in Firefox, at least) via replaceState, then i'll just give up on it for the time being. I'd suggest either finding some other way to set it properly or just changing this back to "".

@QuietMisdreavus
Copy link
Member

With the last force-push, i'm ready to call it. There's still the issue of the page title being wrong, but at this point i'm willing to cut my losses. r=me pending travis.

@QuietMisdreavus
Copy link
Member

Actually, the JS doesn't affect any of the tests that i know of, and it passed tidy, so let's just move along.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 10, 2017

📌 Commit 0d89899 has been approved by QuietMisdreavus

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 11, 2017
…QuietMisdreavus

Fix navbar click while in a search

Fixes rust-lang#45790.
bors added a commit that referenced this pull request Nov 11, 2017
Rollup of 4 pull requests

- Successful merges: #45631, #45812, #45877, #45919
- Failed merges:
@bors bors merged commit 0d89899 into rust-lang:master Nov 11, 2017
@GuillaumeGomez GuillaumeGomez deleted the links-and-search branch November 11, 2017 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants