DevTools: fix text offset when selection ends on expand triangle

third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 429 matching lines @@
 
     return textLines.join('\n');
   }
 
   /**
    * @param {!Element} itemElement
    * @param {!Node} container
    * @param {number} offset
    * @return {number}
    */
   _textOffsetInNode(itemElement, container, offset) {

[einbinder, 2017/05/03 01:07:48]

These names are confusing because the itemElement contains the container. Maybe
call it selection node?

[luoe, 2017/05/03 02:58:39]

Done.

+    // If the container is not a TextNode, we may need to convert a child offset into a character offset.
     if (container.nodeType !== Node.TEXT_NODE) {
       if (offset < container.childNodes.length) {
         container = /** @type {!Node} */ (container.childNodes.item(offset));
         offset = 0;
       } else {
         offset = container.textContent.length;
       }
     }
+    // This method traverses TextNodes until the container is reached. This fails if the container is before the first
+    // TextNode. We can move the container to the next TextNode to avoid this case, which may happen if a text selection
+    // ends on an expand triangle icon.
+    if (container.textContent.length === 0 && container.nodeType !== Node.TEXT_NODE) {

[einbinder, 2017/05/03 01:07:48]

!container.textContent.length? I don't like the idea of selection being in an
empty text node

[luoe, 2017/05/03 02:58:39]

Done.

+      var nextTextNode = container.traverseNextTextNode(itemElement);
+      if (nextTextNode)
+        container = nextTextNode;

[einbinder, 2017/05/03 01:07:48]

This works, but it feels uncomfortable to not have an else here

[luoe, 2017/05/03 02:58:39]

I can see that it seems strange at first on first glance, "If we're supposed to
move the container, wouldn't something break if that fails?"  But we don't
actually need to change the container if there is no next text node.

Perhaps it reads better as
      selectionNode = selectionNode.traverseNextTextNode(itemElement) ||
selectionNode;
?

[einbinder, 2017/05/03 05:15:23]

I guess its fine the way it was.

+    }
 
     var chars = 0;
     var node = itemElement;
     while ((node = node.traverseNextTextNode(itemElement)) && !node.isSelfOrDescendant(container))
       chars += Components.Linkifier.untruncatedNodeText(node).length;
     // If the selection offset is at the end of a link's ellipsis, use the untruncated length as offset.
     var untruncatedContainerLength = Components.Linkifier.untruncatedNodeText(container).length;
     if (offset === 1 && untruncatedContainerLength > offset)
       offset = untruncatedContainerLength;
     return chars + offset;
@@ skipping 132 matching lines @@
 Console.ConsoleViewportElement.prototype = {
   willHide() {},
 
   wasShown() {},
 
   /**
    * @return {!Element}
    */
   element() {},
 };