Bug 1493361 - Add a `urlCropLimit` prop in StringRep. r=Honza.

This new prop allow us to define a maximum length for indivual URLs (as opposed to `cropLimit`, which sets it for the whole text). The URL regex is also modified to be able to linkify messages wrapped in curly quotes. Some tests are added to ensure we handle this prop as expected. Differential Revision: https://phabricator.services.mozilla.com/D29006 --HG-- extra : moz-landing-system : lando
2019-05-13 12:58:24 +00:00 · 2019-05-13 12:58:24 +00:00 · 5638fc9b73
--- a/devtools/client/debugger/packages/devtools-reps/src/reps/rep-utils.js
+++ b/devtools/client/debugger/packages/devtools-reps/src/reps/rep-utils.js
@ -12,7 +12,7 @@ const validProtocols = /(http|https|ftp|data|resource|chrome):/i;
 //                       character so we can include it as part of the text
 //                       preceding the match. We lack look-behind matching.
 //  ^|                   The URL can start at the beginning of the string.
-//  [\s(,;'"`]           Or whitespace or some punctuation that does not imply
+//  [\s(,;'"`“]          Or whitespace or some punctuation that does not imply
 //                       a context which would preclude a URL.
 // )
 //
@ -53,7 +53,7 @@ const validProtocols = /(http|https|ftp|data|resource|chrome):/i;
 //                       (so also '%')
 // )
 // eslint-disable-next-line max-len
-const urlRegex = /(^|[\s(,;'"`])((?:https?:\/\/|www\d{0,3}[.][a-z0-9.\-]{2,249}|[a-z0-9.\-]{2,250}[.][a-z]{2,4}\/)[-\w.!~*'();,/?:@&=+$#%]*)/im;
+const urlRegex = /(^|[\s(,;'"`“])((?:https?:\/\/|www\d{0,3}[.][a-z0-9.\-]{2,249}|[a-z0-9.\-]{2,250}[.][a-z]{2,4}\/)[-\w.!~*'();,/?:@&=+$#%]*)/im;

 // Set of terminators that are likely to have been part of the context rather
 // than part of the URL and so should be uneaten. This is '(', ',', ';', plus
--- a/devtools/client/debugger/packages/devtools-reps/src/reps/string.js
+++ b/devtools/client/debugger/packages/devtools-reps/src/reps/string.js
@ -29,6 +29,7 @@ StringRep.propTypes = {
  escapeWhitespace: PropTypes.bool,
  style: PropTypes.object,
  cropLimit: PropTypes.number.isRequired,
+  urlCropLimit: PropTypes.number,
  member: PropTypes.object,
  object: PropTypes.object.isRequired,
  openLink: PropTypes.func,
@ -42,6 +43,7 @@ function StringRep(props) {
    className,
    style,
    cropLimit,
+    urlCropLimit,
    object,
    useQuotes = true,
    escapeWhitespace = true,
@ -91,12 +93,13 @@ function StringRep(props) {
    if (containsURL(text)) {
      return span(
        config,
-        ...getLinkifiedElements(
+        getLinkifiedElements({
          text,
-          shouldCrop && cropLimit,
+          cropLimit: shouldCrop ? cropLimit : null,
+          urlCropLimit,
          openLink,
          isInContentPage
-        )
+        })
      );
    }

@ -172,14 +175,24 @@ function maybeCropString(opts, text) {
 * Get an array of the elements representing the string, cropped if needed,
 * with actual links.
 *
- * @param {String} text: The actual string to linkify.
- * @param {Integer | null} cropLimit
- * @param {Function} openLink: Function handling the link opening.
- * @param {Boolean} isInContentPage: pass true if the reps is rendered in
- *                                   the content page (e.g. in JSONViewer).
+ * @param {Object} An options object of the following shape:
+ *                 - text {String}: The actual string to linkify.
+ *                 - cropLimit {Integer}: The limit to apply on the whole text.
+ *                 - urlCropLimit {Integer}: The limit to apply on each URL.
+ *                 - openLink {Function} openLink: Function handling the link
+ *                                                 opening.
+ *                 - isInContentPage {Boolean}: pass true if the reps is
+ *                                              rendered in the content page
+ *                                              (e.g. in JSONViewer).
 * @returns {Array<String|ReactElement>}
 */
-function getLinkifiedElements(text, cropLimit, openLink, isInContentPage) {
+function getLinkifiedElements({
+  text,
+  cropLimit,
+  urlCropLimit,
+  openLink,
+  isInContentPage
+}) {
  const halfLimit = Math.ceil((cropLimit - ELLIPSIS.length) / 2);
  const startCropIndex = cropLimit ? halfLimit : null;
  const endCropIndex = cropLimit ? text.length - halfLimit : null;
@ -211,7 +224,7 @@ function getLinkifiedElements(text, cropLimit, openLink, isInContentPage) {
    }

    currentIndex = currentIndex + contentStart;
-    const linkText = getCroppedString(
+    let linkText = getCroppedString(
      useUrl,
      currentIndex,
      startCropIndex,
@ -219,9 +232,20 @@ function getLinkifiedElements(text, cropLimit, openLink, isInContentPage) {
    );

    if (linkText) {
+      if (urlCropLimit && useUrl.length > urlCropLimit) {
+        const urlCropHalf = Math.ceil((urlCropLimit - ELLIPSIS.length) / 2);
+        linkText = getCroppedString(
+          useUrl,
+          0,
+          urlCropHalf,
+          useUrl.length - urlCropHalf
+        );
+      }
+
      items.push(
        a(
          {
+            key: `${useUrl}-${currentIndex}`,
            className: "url",
            title: useUrl,
            draggable: false,
--- a/devtools/client/debugger/packages/devtools-reps/src/reps/tests/string-with-url.js
+++ b/devtools/client/debugger/packages/devtools-reps/src/reps/tests/string-with-url.js
@ -386,7 +386,7 @@ describe("test String with URL", () => {
  it("renders URLs without unrelated characters", () => {
    const text =
      "global(http://example.com) and local(http://example.us)" +
-      " and maybe https://example.fr, https://example.es?";
+      " and maybe https://example.fr, “https://example.cz“, https://example.es?";
    const openLink = jest.fn();
    const element = renderRep(text, {
      openLink,
@ -403,10 +403,84 @@ describe("test String with URL", () => {
    const linkFr = element.find("a").at(2);
    expect(linkFr.prop("href")).toBe("https://example.fr");

-    const linkEs = element.find("a").at(3);
+    const linkCz = element.find("a").at(3);
+    expect(linkCz.prop("href")).toBe("https://example.cz");
+
+    const linkEs = element.find("a").at(4);
    expect(linkEs.prop("href")).toBe("https://example.es");
  });

+  it("renders a cropped URL with urlCropLimit", () => {
+    const xyzUrl = "http://xyz.com/abcdefghijklmnopqrst";
+    const text = `${xyzUrl} is the best`;
+    const openLink = jest.fn();
+    const element = renderRep(text, {
+      openLink,
+      useQuotes: false,
+      urlCropLimit: 20
+    });
+
+    expect(element.text()).toEqual("http://xyz…klmnopqrst is the best");
+    const link = element.find("a").at(0);
+    expect(link.prop("href")).toBe(xyzUrl);
+    expect(link.prop("title")).toBe(xyzUrl);
+  });
+
+  it("renders multiple cropped URL", () => {
+    const xyzUrl = "http://xyz.com/abcdefghijklmnopqrst";
+    const abcUrl = "http://abc.com/abcdefghijklmnopqrst";
+    const text = `${xyzUrl} is lit, not ${abcUrl}`;
+    const openLink = jest.fn();
+    const element = renderRep(text, {
+      openLink,
+      useQuotes: false,
+      urlCropLimit: 20
+    });
+
+    expect(element.text()).toEqual(
+      "http://xyz…klmnopqrst is lit, not http://abc…klmnopqrst"
+    );
+
+    const links = element.find("a");
+    const xyzLink = links.at(0);
+    expect(xyzLink.prop("href")).toBe(xyzUrl);
+    expect(xyzLink.prop("title")).toBe(xyzUrl);
+    const abc = links.at(1);
+    expect(abc.prop("href")).toBe(abcUrl);
+    expect(abc.prop("title")).toBe(abcUrl);
+  });
+
+  it("renders full URL if smaller than cropLimit", () => {
+    const xyzUrl = "http://example.com/";
+
+    const openLink = jest.fn();
+    const element = renderRep(xyzUrl, {
+      openLink,
+      useQuotes: false,
+      urlCropLimit: 20
+    });
+
+    expect(element.text()).toEqual(xyzUrl);
+    const link = element.find("a").at(0);
+    expect(link.prop("href")).toBe(xyzUrl);
+    expect(link.prop("title")).toBe(xyzUrl);
+  });
+
+  it("renders cropped URL followed by cropped string with urlCropLimit", () => {
+    const text = "http://example.fr abcdefghijkl";
+    const openLink = jest.fn();
+    const element = renderRep(text, {
+      openLink,
+      useQuotes: false,
+      cropLimit: 20
+    });
+
+    expect(element.text()).toEqual("http://exa…cdefghijkl");
+    const linkFr = element.find("a").at(0);
+    expect(linkFr.prop("href")).toBe("http://example.fr");
+    expect(linkFr.prop("title")).toBe("http://example.fr");
+  });
+
  it("does not render a link if the URL has no scheme", () => {
    const url = "example.com";
    const element = renderRep(url, { useQuotes: false });
--- a/devtools/client/shared/components/reps/reps.js
+++ b/devtools/client/shared/components/reps/reps.js
@ -3747,7 +3747,7 @@ const validProtocols = /(http|https|ftp|data|resource|chrome):/i;
 //                       character so we can include it as part of the text
 //                       preceding the match. We lack look-behind matching.
 //  ^|                   The URL can start at the beginning of the string.
-//  [\s(,;'"`]           Or whitespace or some punctuation that does not imply
+//  [\s(,;'"`“]          Or whitespace or some punctuation that does not imply
 //                       a context which would preclude a URL.
 // )
 //
@ -3788,7 +3788,7 @@ const validProtocols = /(http|https|ftp|data|resource|chrome):/i;
 //                       (so also '%')
 // )
 // eslint-disable-next-line max-len
-const urlRegex = /(^|[\s(,;'"`])((?:https?:\/\/|www\d{0,3}[.][a-z0-9.\-]{2,249}|[a-z0-9.\-]{2,250}[.][a-z]{2,4}\/)[-\w.!~*'();,/?:@&=+$#%]*)/im;
+const urlRegex = /(^|[\s(,;'"`“])((?:https?:\/\/|www\d{0,3}[.][a-z0-9.\-]{2,249}|[a-z0-9.\-]{2,250}[.][a-z]{2,4}\/)[-\w.!~*'();,/?:@&=+$#%]*)/im;

 // Set of terminators that are likely to have been part of the context rather
 // than part of the URL and so should be uneaten. This is '(', ',', ';', plus
@ -4396,6 +4396,7 @@ StringRep.propTypes = {
  escapeWhitespace: PropTypes.bool,
  style: PropTypes.object,
  cropLimit: PropTypes.number.isRequired,
+  urlCropLimit: PropTypes.number,
  member: PropTypes.object,
  object: PropTypes.object.isRequired,
  openLink: PropTypes.func,
@ -4409,6 +4410,7 @@ function StringRep(props) {
    className,
    style,
    cropLimit,
+    urlCropLimit,
    object,
    useQuotes = true,
    escapeWhitespace = true,
@ -4450,7 +4452,13 @@ function StringRep(props) {

  if (!isLong) {
    if (containsURL(text)) {
-      return span(config, ...getLinkifiedElements(text, shouldCrop && cropLimit, openLink, isInContentPage));
+      return span(config, getLinkifiedElements({
+        text,
+        cropLimit: shouldCrop ? cropLimit : null,
+        urlCropLimit,
+        openLink,
+        isInContentPage
+      }));
    }

    // Cropping of longString has been handled before formatting.
@ -4520,14 +4528,24 @@ function maybeCropString(opts, text) {
 * Get an array of the elements representing the string, cropped if needed,
 * with actual links.
 *
- * @param {String} text: The actual string to linkify.
- * @param {Integer | null} cropLimit
- * @param {Function} openLink: Function handling the link opening.
- * @param {Boolean} isInContentPage: pass true if the reps is rendered in
- *                                   the content page (e.g. in JSONViewer).
+ * @param {Object} An options object of the following shape:
+ *                 - text {String}: The actual string to linkify.
+ *                 - cropLimit {Integer}: The limit to apply on the whole text.
+ *                 - urlCropLimit {Integer}: The limit to apply on each URL.
+ *                 - openLink {Function} openLink: Function handling the link
+ *                                                 opening.
+ *                 - isInContentPage {Boolean}: pass true if the reps is
+ *                                              rendered in the content page
+ *                                              (e.g. in JSONViewer).
 * @returns {Array<String|ReactElement>}
 */
-function getLinkifiedElements(text, cropLimit, openLink, isInContentPage) {
+function getLinkifiedElements({
+  text,
+  cropLimit,
+  urlCropLimit,
+  openLink,
+  isInContentPage
+}) {
  const halfLimit = Math.ceil((cropLimit - ELLIPSIS.length) / 2);
  const startCropIndex = cropLimit ? halfLimit : null;
  const endCropIndex = cropLimit ? text.length - halfLimit : null;
@ -4557,10 +4575,16 @@ function getLinkifiedElements(text, cropLimit, openLink, isInContentPage) {
    }

    currentIndex = currentIndex + contentStart;
-    const linkText = getCroppedString(useUrl, currentIndex, startCropIndex, endCropIndex);
+    let linkText = getCroppedString(useUrl, currentIndex, startCropIndex, endCropIndex);

    if (linkText) {
+      if (urlCropLimit && useUrl.length > urlCropLimit) {
+        const urlCropHalf = Math.ceil((urlCropLimit - ELLIPSIS.length) / 2);
+        linkText = getCroppedString(useUrl, 0, urlCropHalf, useUrl.length - urlCropHalf);
+      }
+
      items.push(a({
+        key: `${useUrl}-${currentIndex}`,
        className: "url",
        title: useUrl,
        draggable: false,