Fix issue with navigating in firefox/safari

[timroes]

Summary

Fix #65090

This fixes an issue in Visualize that you cannot navigate to create new visualizations (that don't require an index pattern, like TSVB or Vega). The reason for this issue happening is that the history object that we use in multiple methods, and also in removeQueryParam, won't reflect the new URL instantly in Firefox and Safari, since they fire the hashchange event a bit delayed. So the removeQueryParam method will still see the /visualize URL (and not /visualize/create as it should when creating a new visualization), and then just remove a parameter from it (or not) and navigate to it, i.e. it navigates back to the table where it's coming from, because it never saw the new URL.

The long term fix for this is described in #65161, but will require significant larger changes, which are out of the scope of a fix for 7.7.0.

This fix will simply make sure we're waiting one execution slot before finishing the navigation to /visualize/create (and only that page). This will cause the Editor Angular Controller to be instantiated one execution slot later, in which case history always reflects the correct URL (/visualize/create). Since we're only waiting one execution slot (and no specific amount of ms), there won't be any noticeable delay introduced by that change.

Regarding tests: I have no idea how we could properly test this, since this is based on specific browser behavior in Firefox/Safari, that we cannot easily catch in any unit test and in a functional test only in Firefox. Thus I think we cannot provide any proper test for this PR, but the correct "test" for this is to work towards enabling the functional visualization tests in the firefox test suite.

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
• This was checked for cross-browser compatibility, including a check against IE11

For maintainers

• [ ] This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/kibana-app (Team:KibanaApp)

[bhavyarm]

Tested this on - on create visualizations and from create new on dashboards and on both safari and firefox latest. I picked creating TSVB, vega, timelion, area, pie, gauge, and markdown using visualize in both browsers & randomized them. Also default and non-default spaces. AFAIK - nothing broke in chrome because of this fix. Needs IE testing.

[lukeelmers]

Code changes here LGTM; thanks for linking directly to the follow-up task in the code 👍

Tested this PR locally (macOS) in Chrome, Safari, Firefox, and am no longer able to reproduce the issue (I'm able to consistently reproduce on master). Also tried a handful of vis types and all seems to be behaving normally.

Regarding tests: I have no idea how we could properly test this, since this is based on specific browser behavior in Firefox/Safari, that we cannot easily catch in any unit test and in a functional test only in Firefox. Thus I think we cannot provide any proper test for this PR, but the correct "test" for this is to work towards enabling the functional visualization tests in the firefox test suite.

I agree with this assessment; I'm not really sure how we'd automate a test for this without browser-specific functional tests for visualizations.

[wylieconlon]

Edit: I needed to reread some docs on Chained Promises.

Won't this .catch no longer catch errors from the getResolvedResults function, since the delay function can never throw an error? This looks like it could inadvertently cause our error handling behavior to be different

[wylieconlon]

I've compared the behavior of the 7.x branch against this PR in Chrome, Firefox, and Safari, and wasn't able to consistently reproduce the bug in any browser. Your explanation makes sense and is definitely a workaround that I've used with the angular router in past projects, so I think this is a fine change. The behavior definitely doesn't get worse with this change.

My comment about error handling logic was based on my testing with unrealistic error messages: It looks like we handle errors from the SavedObjectLoader in a custom way.

[ThomThomson]

This fix LGTM,
Tested via creating timelion, vega, and TSVB charts from the visualizations listing.

Tested on:
Chrome on osx
Firefox on osx
Safari on osx

Waiting on updates in order to test with IE 11

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: b7a1c91

History

• 💚 Build #45223 succeeded a8204b4

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream