Skip to content

Re-enable history api on file:// protocol - #57504

Merged
bors merged 1 commit into
rust-lang:masterfrom
GuillaumeGomez:re-enable-history
Feb 7, 2019
Merged

Re-enable history api on file:// protocol#57504
bors merged 1 commit into
rust-lang:masterfrom
GuillaumeGomez:re-enable-history

Conversation

@GuillaumeGomez

@GuillaumeGomezGuillaumeGomez commented Jan 10, 2019

Copy link
Copy Markdown
Member

Fixes#57135.

I tested locally on chrome (since it was the browser having issues with history management on file:// protocol) and it worked fine so I guess we can re-enable it.

r? @QuietMisdreavus

@ecstatic-morse

ecstatic-morse commented Jan 10, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Using history.pushState on a page loaded from file:// throws a SecurityError in Firefox 64, so I don't think you should unconditionally enable this.

As far as I can tell, it looks like history.pushState on a page loaded from file:// throws a SecurityError in Firefox 64 if the pages have different origin. Since opening the search window just adds a query parameter, the calls pushState should be fine for most modern browsers, but you might want to add a comment to this effect.

@steveklabnik

steveklabnik commented Jan 10, 2019 via email

Copy link
Copy Markdown
Contributor

@GuillaumeGomez

GuillaumeGomez commented Jan 11, 2019

Copy link
Copy Markdown
MemberAuthor

The problem was supposed to be only on chrome. My firefox worked correctly though.

@QuietMisdreavus

QuietMisdreavus commented Jan 11, 2019

Copy link
Copy Markdown
Contributor

As far as I can tell, it looks like history.pushState on a page loaded from file:// throws a SecurityError in Firefox 64 if the pages have different origin. Since opening the search window just adds a query parameter, the calls pushState should be fine for most modern browsers, but you might want to add a comment to this effect.

This seems to make it okay? I think we only use the history API when loading up the search results, so it may work out.

@GuillaumeGomez

GuillaumeGomez commented Jan 11, 2019

Copy link
Copy Markdown
MemberAuthor

Normally yes.

@stokhosstokhos added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 14, 2019
@TimNNTimNN added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2019
@QuietMisdreavus

QuietMisdreavus commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

I double-checked main.js to make sure we only used the history API (or at least, only this detection) when modifying the query string or hash of the URL, and not the file. Because of that, i'm okay merging this.

@bors r+ rollup

@bors

bors commented Feb 7, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 830b3b8 has been approved by QuietMisdreavus

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2019
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 7, 2019
@GuillaumeGomez
Rollup merge ofrust-lang#57504- GuillaumeGomez:re-enable-history, r…
55c0922
…=QuietMisdreavus
Re-enable history api on file:// protocol
Fixesrust-lang#57135.
I tested locally on chrome (since it was the browser having issues with history management on `file://` protocol) and it worked fine so I guess we can re-enable it.
r? @QuietMisdreavus
@GuillaumeGomezGuillaumeGomez mentioned this pull request Feb 7, 2019
Merged
bors added a commit that referenced this pull request Feb 7, 2019
@bors
Auto merge of#58266- GuillaumeGomez:rollup, r=GuillaumeGomez
d173180
Rollup of 11 pull requests
Successful merges:
- #57504 (Re-enable history api on file:// protocol)
- #57848 (Generate a documentation page for core::mem::transmute.)
- #57884 (Update minifier version)
- #57954 (rustdoc: remove blank unstable spans)
- #58028 (Fix image link in the settings menu)
- #58033 (rustdoc: wrap stability tags in colored spans)
- #58086 ([rustdoc] Improve file list display)
- #58143 (Sort elements in the sidebar)
- #58146 (Prevent automatic collapse of methods impl blocks)
- #58150 (Don't apply impl block collapse rules to trait impls)
- #58185 (Remove images' url to make it work even without internet connection)
Failed merges:
r? @ghost
@bors
bors merged commit 830b3b8 into rust-lang:masterFeb 7, 2019
@GuillaumeGomez
GuillaumeGomez deleted the re-enable-history branch February 8, 2019 00:36
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@QuietMisdreavusQuietMisdreavus

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@GuillaumeGomez@ecstatic-morse@steveklabnik@QuietMisdreavus@bors@TimNN@stokhos