[ML] Fix regression with some links not opening in new tab #80785

qn895 · 2020-10-15T23:40:20Z

Summary

This PR fixes #80639 where some links not opening in a new tab. Changes include:

Anomaly detection View Series and View now open in new tab
In 7.9, clicking on the link like View Series or View in the Anomaly Explorer would automatically open the page in another tab. [ML] Migrate internal urls to non-hash paths #76735 later makes it so that it redirect to the new page in the same tab. This PR changes the behavior back to work like how it was in 7.9.
Fix an issue with the calendar link not working correctly in management page

Change the EuiCard to behave as links instead of button in Data Visualizer results page
Changes from [ML] avoid full page reload for links following CSV import #79539 makes it so that the Data Visualizer won't do a full page reload, but the change makes it cards are no longer linkable. This PR makes it so that these cards act as links, and recent code splitting efforts have made the wait time much less.

Checklist

This was checked for cross-browser compatibility

…ting to new page

This reverts commit 09f8421a0562675fafdada78304417e9e64e656f.

elasticmachine · 2020-10-15T23:40:23Z

Pinging @elastic/ml-ui (:ml)

darnautov · 2020-10-16T08:02:55Z

x-pack/plugins/ml/public/application/components/anomalies_table/links_menu.js

-    await navigateToApp(PLUGIN_ID, {
-      path: singleMetricViewerLink,
-    });
+    window.open(singleMetricViewerLink, '_blank');


shall we use MlHref instead of window.open?

We'll keep this one here for now for 7.10.0.

darnautov · 2020-10-16T08:04:36Z

x-pack/plugins/ml/public/application/components/links/ml_href.tsx

+  return !!(event.metaKey || event.altKey || event.ctrlKey || event.shiftKey);
+}
+
+export const MlHref = ({


This component deserves a brief description

darnautov · 2020-10-16T08:12:53Z

x-pack/plugins/ml/public/application/components/links/ml_href.tsx

+      onClick={(event) => {
+        try {
+          if (onClick) onClick(event);
+        } catch (ex) {
+          event.preventDefault();
+          throw ex;
+        }
+
+        if (
+          !event.defaultPrevented && // onClick prevented default
+          event.button === 0 && // ignore everything but left clicks
+          (!target || target === '_self') && // let browser handle "target=_blank" etc.
+          !isModifiedEvent(event) // ignore clicks with modifier keys
+        ) {
+          event.preventDefault();
+          navigateToPath(href);
+          return false;
+        }
+        return true;
+      }}


I think we should call event. preventDefault only once at the beginning of the function with isModifiedEvent check and target="_blank" maybe. But I believe we should not use target=_blank inside of Kibana. There is only one exception - external URLs, hence you can omit the target check in this component because it's not applicable.

There is no need to return true or false. I recall it from old times when preventDefault wasn't a standard.

The onClick callback that comes with props can cause potential issues, i.e. update state on an unmounted component. Making it Promise based should help.

darnautov · 2020-10-16T08:29:43Z

x-pack/plugins/ml/public/application/components/links/ml_href.tsx

+  return (
+    <a
+      target={target}
+      href={basePath.prepend(`/app/ml/${href}`)}


I think this component should also support URLs besides the ML plugin.

darnautov · 2020-10-16T08:32:14Z

x-pack/plugins/ml/public/application/components/links/ml_href.tsx

+  target,
+  onClick,
+  children,
+  ...rest


Not sure if ...rest should be here because you already destructured all the props.

This ...rest is here if the user wants to pass on extra props for the tag like 'className' or style.

darnautov · 2020-10-16T08:38:57Z

...s/ml/public/application/datavisualizer/file_based/components/results_links/results_links.tsx

-      path: `/kibana/indexPatterns${createIndexPattern ? `/patterns/${indexPatternId}` : ''}`,
-    });
-  }
-
  return (
    <EuiFlexGroup gutterSize="l">
      {createIndexPattern && discoverLink && (


do you need to add a check for all the other links as well?

Updated here 0064254

x-pack/plugins/ml/public/application/components/links/ml_href.tsx

pheyos · 2020-10-16T12:09:10Z

I've pulled the changes and tested it locally, but the view series links and data visualizer cards could still not be opened in a new tab. Maybe something is wrong with my test setup - I'll try again later.

peteharverson · 2020-10-16T13:09:21Z

@pheyos I was able to open the file data visualizer card links into new tabs (right click on text on the card worked for me). The 'View series' link will always open in a new tab with this change - I think this is preferable as a quick fix to opening in the same tab, which makes the behavior consistent with 7.9.

pheyos · 2020-10-16T14:57:19Z

@peteharverson I've set up the branch again and now it's working as you described.

But one thing that's not working correctly is: when I follow a view series link in the anomaly explorer it's working ok until I close the new opened tab. In that case the old tab freezes and doesn't come back working for me. Is it only me or is anyone else seeing the same?

…use EuiLink with

…ssion

qn895 · 2020-10-16T16:03:10Z

@peteharverson I've set up the branch again and now it's working as you described.

But one thing that's not working correctly is: when I follow a view series link in the anomaly explorer it's working ok until I close the new opened tab. In that case the old tab freezes and doesn't come back working for me. Is it only me or is anyone else seeing the same?

I was able to reproduce this but not reliably. How weird! Looking into it some more.

peteharverson · 2020-10-16T16:12:28Z

@pheyos the issue you are seeing with the old tab freezing is a known issue that @darnautov has spent some time looking into, and is not related to the changes in this PR. It only occurs when running Kibana in dev mode as far as we know.

peteharverson

Tested and LGTM

jgowdyelastic · 2020-10-16T16:37:27Z

...k/plugins/ml/public/application/jobs/jobs_list/components/job_details/extract_job_details.js

@@ -61,7 +61,9 @@ export function extractJobDetails(job) {
  if (job.calendars) {
    calendars.items = job.calendars.map((c) => [
      '',
-      <Link to={`/settings/calendars_list/edit_calendar/${c}?_g=()`}>{c}</Link>,
+      <EuiLink href={basePath.prepend(`/app/ml/settings/calendars_list/edit_calendar/${c}?_g=()`)}>


rather than using Link or EuiLink here, is there a better way to create this link?
i'm asking because we don't need basePath anywhere else in this PR, so why here?

I think moving forward we will have to use the url with basePath for all the href. The EuiLink work here without causing a full page reload because we are wrapping the ml application inside RedirectAppLinks. Alternatively, we can use it as a button and navigate the user to the new page but that means user will no longer be able to ctrl+click or right click to open in new tab.

RedirectAppLinks intercepts any tag and override the onClick behavior to use navigatetoUrl, and therefore requires the href to have the full base path https://github.com/smith/kibana/blob/765a70f7cc2583d394b96015a6fd7f5ab44662ba/docs/developer/best-practices/navigation.asciidoc#L21

Also, in this particular component the basePath is needed so the link will work in the management section. If not the href would be like abc/app/management/insightsAndAlerting/jobsListLink/settings/calendars_list/edit_calendar/nov4?_g=() which is invalid. So that would mean we have to check if the table is being rendered in the ml plugin or the management section, and either prepend or not prepend the path for it to work properly.

qn895 · 2020-10-19T02:06:12Z

@elasticmachine merge upstream

qn895 · 2020-10-19T03:15:16Z

@elasticmachine merge upstream

kibanamachine · 2020-10-19T04:50:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: df14a3a

Metrics [docs]

async chunks size

id	before	after	diff
`ml`	7.2MB	7.2MB	+3.2KB

History

💚 Build #82207 succeeded ac8ffcc
💚 Build #82070 succeeded b5418b6

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

jgowdyelastic

LGTM

…0785) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…0785) (#80981) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

) (#80980) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

qn895 added 7 commits October 15, 2020 16:53

[ML] Fix so View series will open page in new tab instead of re-direc…

65e86d4

…ting to new page

[ML] Open link in annotation in new tab

257835a

[ML] Fix links in data visualizer to allow to open in new tab

b8052fe

[ML] Fix broken calendar link in management

36cc49a

Revert "[ML] Open link in annotation in new tab"

9e68a05

This reverts commit 09f8421a0562675fafdada78304417e9e64e656f.

[ML] Fix single explorer chart link

66e775a

[ML] Clean up console

b5418b6

qn895 added release_note:fix :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 v7.10.0 labels Oct 15, 2020

qn895 requested review from pheyos, darnautov and peteharverson October 15, 2020 23:40

qn895 requested a review from a team as a code owner October 15, 2020 23:40

qn895 self-assigned this Oct 15, 2020

darnautov reviewed Oct 16, 2020

View reviewed changes

x-pack/plugins/ml/public/application/components/links/ml_href.tsx Outdated Show resolved Hide resolved

qn895 added 2 commits October 16, 2020 10:35

[ML] Add checks to data visualizer results, change jobs_list_page to …

0064254

…use EuiLink with

Merge remote-tracking branch 'upstream/master' into ml-fix-link-regre…

ac8ffcc

…ssion

peteharverson approved these changes Oct 16, 2020

View reviewed changes

peteharverson added the v7.11.0 label Oct 16, 2020

jgowdyelastic reviewed Oct 16, 2020

View reviewed changes

Merge branch 'master' into ml-fix-link-regression

df14a3a

jgowdyelastic approved these changes Oct 19, 2020

View reviewed changes

qn895 merged commit f4c596d into elastic:master Oct 19, 2020

qn895 deleted the ml-fix-link-regression branch October 19, 2020 13:36

qn895 mentioned this pull request Oct 19, 2020

[7.x] [ML] Fix regression with some links not opening in new tab (#80785) #80980

Merged

qn895 added a commit to qn895/kibana that referenced this pull request Oct 19, 2020

[ML] Fix regression with some links not opening in new tab (elastic#8…

1bcdf4e

…0785) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

qn895 mentioned this pull request Oct 19, 2020

[7.10] [ML] Fix regression with some links not opening in new tab (#80785) #80981

Merged

qn895 added a commit to qn895/kibana that referenced this pull request Oct 19, 2020

[ML] Fix regression with some links not opening in new tab (elastic#8…

d537836

…0785) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Fix regression with some links not opening in new tab #80785

[ML] Fix regression with some links not opening in new tab #80785

qn895 commented Oct 15, 2020

elasticmachine commented Oct 15, 2020

darnautov Oct 16, 2020

qn895 Oct 16, 2020

darnautov Oct 16, 2020

darnautov Oct 16, 2020

darnautov Oct 16, 2020

darnautov Oct 16, 2020

qn895 Oct 16, 2020

darnautov Oct 16, 2020

qn895 Oct 16, 2020

pheyos commented Oct 16, 2020

peteharverson commented Oct 16, 2020

pheyos commented Oct 16, 2020

qn895 commented Oct 16, 2020

peteharverson commented Oct 16, 2020

peteharverson left a comment

jgowdyelastic Oct 16, 2020

qn895 Oct 16, 2020 •

edited

Loading

qn895 commented Oct 19, 2020

qn895 commented Oct 19, 2020

kibanamachine commented Oct 19, 2020

jgowdyelastic left a comment

[ML] Fix regression with some links not opening in new tab #80785

[ML] Fix regression with some links not opening in new tab #80785

Conversation

qn895 commented Oct 15, 2020

Summary

Checklist

elasticmachine commented Oct 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pheyos commented Oct 16, 2020

peteharverson commented Oct 16, 2020

pheyos commented Oct 16, 2020

qn895 commented Oct 16, 2020

peteharverson commented Oct 16, 2020

peteharverson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qn895 Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

qn895 commented Oct 19, 2020

qn895 commented Oct 19, 2020

kibanamachine commented Oct 19, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

History

jgowdyelastic left a comment

Choose a reason for hiding this comment

qn895 Oct 16, 2020 •

edited

Loading