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

Back button navigation fix 31238 #32372

Merged
merged 4 commits into from
Mar 5, 2019

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Mar 3, 2019

Summary

Fixed issue 31238, where back navigation would get stuck due to bad URL encoding.

Includes a functional test that was used to reproduce the issue.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lizozom lizozom added bug Fixes for quality problems that affect the customer experience Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 v7.2.0 labels Mar 4, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@lizozom lizozom self-assigned this Mar 4, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM!

@lizozom lizozom merged commit ace80d1 into elastic:master Mar 5, 2019
lizozom added a commit to lizozom/kibana that referenced this pull request Mar 5, 2019
* initial fix for navigation issue + Spencer
* added navigation test for issue elastic#31238 
* added more validations to the navigate back test
lizozom added a commit that referenced this pull request Mar 5, 2019
* initial fix for navigation issue + Spencer
* added navigation test for issue #31238 
* added more validations to the navigate back test
@sorenlouv
Copy link
Member

sorenlouv commented Mar 7, 2019

Hey @lizozom,

Nice work fixing the back button 👍
Unfortunately this also introduced a little side-effect which is affecting all urls in Kibana.

Previously the following query:

?rangeFrom=15%3A55&path=a/b/c

Would be converted by Kibana/Angular to:

?rangeFrom=15:55&path=a%2Fb%2Fc

Note how the rangeFrom value was decoded, and path value was encoded. Super bizarre behaviour already, so really don't blame you. Touching url handling in Kibana is a minefield! 💣

However, after this PR was merged, the same query:

?rangeFrom=15%3A55&path=a/b/c

Is now converted into

?rangeFrom=15:55&path=a%2Fb%2Fc#%2Fhome%3FrangeFrom=15:55&path=a%252Fb%252Fc

@jasonrhodes
Copy link
Member

I made an issue for this here: #32721

@sorenlouv
Copy link
Member

I'm thinking an initial solution could be to guard everything inside $locationChangeStart with a check so the redirect is only done for explicit plugins, eg Discover, Visualize, Management.

This way it won't affect plugins that uses other routing mechanism and url schemes.

spalger pushed a commit that referenced this pull request Mar 13, 2019
* initial fix for navigation issue + Spencer
* added navigation test for issue #31238 
* added more validations to the navigate back test

(cherry picked from commit ace80d1)
@spalger spalger added the v7.0.0 label Mar 13, 2019
@spalger
Copy link
Contributor

spalger commented Mar 13, 2019

7.0: 922eb1e
7.x/7.1: 89f45f2

@lizozom lizozom deleted the back-button-navigation-fix-31238 branch November 14, 2019 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants