Bug 1493361 - Add a `urlCropLimit` prop in StringRep. r=Honza.
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Mon, 13 May 2019 12:58:24 +0000
changeset 532423 395e27a912b3b6aea3c27ccdb8a0028c23abf223
parent 532422 3fc09d5a4f225b2164b2a4ade0c2b1545f999903
child 532424 b87cdb4550833374f77f1bc5163e554552fb35f9
push id11268
push usercsabou@mozilla.com
push dateTue, 14 May 2019 15:24:22 +0000
treeherdermozilla-beta@5fb7fcd568d6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersHonza
bugs1493361
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
devtools/client/debugger/packages/devtools-reps/src/reps/rep-utils.js
devtools/client/debugger/packages/devtools-reps/src/reps/string.js
devtools/client/debugger/packages/devtools-reps/src/reps/tests/string-with-url.js
devtools/client/shared/components/reps/reps.js
--- a/devtools/client/debugger/packages/devtools-reps/src/reps/rep-utils.js
+++ b/devtools/client/debugger/packages/devtools-reps/src/reps/rep-utils.js
@@ -7,17 +7,17 @@ const validProtocols = /(http|https|ftp|
 
 // URL Regex, common idioms:
 //
 // Lead-in (URL):
 // (                     Capture because we need to know if there was a lead-in
 //                       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.
 // )
 //
 // We do not need a trailing look-ahead because our regex's will terminate
 // because they run out of characters they can eat.
 
 // What we do not attempt to have the regexp do:
 // - Avoid trailing '.' and ')' characters.  We let our greedy match absorb
@@ -48,17 +48,17 @@ const validProtocols = /(http|https|ftp|
 //                       found here: http://www.iana.org/domains/root/db
 //  )
 //  [-\w.!~*'();,/?:@&=+$#%]*
 //                       path onwards. We allow the set of characters that
 //                       encodeURI does not escape plus the result of escaping
 //                       (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
 // quotes and question end-ing punctuation and the potential permutations with
 // parentheses (english-specific).
 const uneatLastUrlCharsRegex = /(?:[),;.!?`'"]|[.!?]\)|\)[.!?])$/;
 
 const ELLIPSIS = "\u2026";
--- a/devtools/client/debugger/packages/devtools-reps/src/reps/string.js
+++ b/devtools/client/debugger/packages/devtools-reps/src/reps/string.js
@@ -24,29 +24,31 @@ const { a, span } = dom;
 /**
  * Renders a string. String value is enclosed within quotes.
  */
 StringRep.propTypes = {
   useQuotes: PropTypes.bool,
   escapeWhitespace: PropTypes.bool,
   style: PropTypes.object,
   cropLimit: PropTypes.number.isRequired,
+  urlCropLimit: PropTypes.number,
   member: PropTypes.object,
   object: PropTypes.object.isRequired,
   openLink: PropTypes.func,
   className: PropTypes.string,
   title: PropTypes.string,
   isInContentPage: PropTypes.bool
 };
 
 function StringRep(props) {
   const {
     className,
     style,
     cropLimit,
+    urlCropLimit,
     object,
     useQuotes = true,
     escapeWhitespace = true,
     member,
     openLink,
     title,
     isInContentPage
   } = props;
@@ -86,22 +88,23 @@ function StringRep(props) {
     actor: object.actor,
     title
   });
 
   if (!isLong) {
     if (containsURL(text)) {
       return span(
         config,
-        ...getLinkifiedElements(
+        getLinkifiedElements({
           text,
-          shouldCrop && cropLimit,
+          cropLimit: shouldCrop ? cropLimit : null,
+          urlCropLimit,
           openLink,
           isInContentPage
-        )
+        })
       );
     }
 
     // Cropping of longString has been handled before formatting.
     text = maybeCropString(
       {
         isLong,
         shouldCrop,
@@ -167,24 +170,34 @@ function maybeCropString(opts, text) {
 
   return shouldCrop ? rawCropString(text, cropLimit) : 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;
 
   const items = [];
   let currentIndex = 0;
   let contentStart;
   while (true) {
@@ -206,27 +219,38 @@ function getLinkifiedElements(text, crop
     // URL.
     let useUrl = url[2];
     const uneat = uneatLastUrlCharsRegex.exec(useUrl);
     if (uneat) {
       useUrl = useUrl.substring(0, uneat.index);
     }
 
     currentIndex = currentIndex + contentStart;
-    const linkText = getCroppedString(
+    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,
             // Because we don't want the link to be open in the current
             // panel's frame, we only render the href attribute if `openLink`
             // exists (so we can preventDefault) or if the reps will be
             // displayed in content page (e.g. in the JSONViewer).
             href: openLink || isInContentPage ? useUrl : null,
--- 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
@@ -381,37 +381,111 @@ describe("test String with URL", () => {
     const linkFr = element.find("a").at(0);
     expect(linkFr.prop("href")).toBe("http://example.fr");
     expect(linkFr.prop("title")).toBe("http://example.fr");
   });
 
   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,
       useQuotes: false
     });
 
     expect(element.text()).toEqual(text);
     const linkCom = element.find("a").at(0);
     expect(linkCom.prop("href")).toBe("http://example.com");
 
     const linkUs = element.find("a").at(1);
     expect(linkUs.prop("href")).toBe("http://example.us");
 
     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 });
     expect(element.text()).toEqual(url);
     expect(element.find("a").exists()).toBeFalsy();
   });
 
   it("does not render a link if the URL has an invalid scheme", () => {
--- a/devtools/client/shared/components/reps/reps.js
+++ b/devtools/client/shared/components/reps/reps.js
@@ -3742,17 +3742,17 @@ const validProtocols = /(http|https|ftp|
 
 // URL Regex, common idioms:
 //
 // Lead-in (URL):
 // (                     Capture because we need to know if there was a lead-in
 //                       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.
 // )
 //
 // We do not need a trailing look-ahead because our regex's will terminate
 // because they run out of characters they can eat.
 
 // What we do not attempt to have the regexp do:
 // - Avoid trailing '.' and ')' characters.  We let our greedy match absorb
@@ -3783,17 +3783,17 @@ const validProtocols = /(http|https|ftp|
 //                       found here: http://www.iana.org/domains/root/db
 //  )
 //  [-\w.!~*'();,/?:@&=+$#%]*
 //                       path onwards. We allow the set of characters that
 //                       encodeURI does not escape plus the result of escaping
 //                       (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
 // quotes and question end-ing punctuation and the potential permutations with
 // parentheses (english-specific).
 const uneatLastUrlCharsRegex = /(?:[),;.!?`'"]|[.!?]\)|\)[.!?])$/;
 
 const ELLIPSIS = "\u2026";
@@ -4391,29 +4391,31 @@ const { a, span } = dom;
 /**
  * Renders a string. String value is enclosed within quotes.
  */
 StringRep.propTypes = {
   useQuotes: PropTypes.bool,
   escapeWhitespace: PropTypes.bool,
   style: PropTypes.object,
   cropLimit: PropTypes.number.isRequired,
+  urlCropLimit: PropTypes.number,
   member: PropTypes.object,
   object: PropTypes.object.isRequired,
   openLink: PropTypes.func,
   className: PropTypes.string,
   title: PropTypes.string,
   isInContentPage: PropTypes.bool
 };
 
 function StringRep(props) {
   const {
     className,
     style,
     cropLimit,
+    urlCropLimit,
     object,
     useQuotes = true,
     escapeWhitespace = true,
     member,
     openLink,
     title,
     isInContentPage
   } = props;
@@ -4445,17 +4447,23 @@ function StringRep(props) {
     className,
     style,
     actor: object.actor,
     title
   });
 
   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.
     text = maybeCropString({
       isLong,
       shouldCrop,
       cropLimit
     }, text);
@@ -4515,24 +4523,34 @@ function maybeCropString(opts, text) {
 
   return shouldCrop ? rawCropString(text, cropLimit) : 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;
 
   const items = [];
   let currentIndex = 0;
   let contentStart;
   while (true) {
@@ -4552,20 +4570,26 @@ function getLinkifiedElements(text, crop
     // URL.
     let useUrl = url[2];
     const uneat = uneatLastUrlCharsRegex.exec(useUrl);
     if (uneat) {
       useUrl = useUrl.substring(0, uneat.index);
     }
 
     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,
         // Because we don't want the link to be open in the current
         // panel's frame, we only render the href attribute if `openLink`
         // exists (so we can preventDefault) or if the reps will be
         // displayed in content page (e.g. in the JSONViewer).
         href: openLink || isInContentPage ? useUrl : null,