Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2568983003:
Add ability to linkify substituted string
|
Created:
4 years ago by karabur Modified:
3 years, 10 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ability to linkify substituted string
Currently, if string contains substitutions, only separate parts of the string
are replaced with links.
This patch change logic to linkify whole string despite is it generated by substitutions or not
BUG=668591
Patch Set 1 #
Total comments: 11
Patch Set 2 : More generic approach to linkify, covering most of cases #Patch Set 3 : Comments addressed #
Total comments: 12
Messages
Total messages: 24 (9 generated)
Description was changed from ========== Add ability to linkify substituted string Currently, if string contains substitutions, only separate parts of the string are replaced with links. This patch change logic to linkify whole string despite is it generated by substitutions or not BUG=668591 ========== to ========== Add ability to linkify substituted string Currently, if string contains substitutions, only separate parts of the string are replaced with links. This patch change logic to linkify whole string despite is it generated by substitutions or not BUG=668591 ==========
anton.ulianov@gmail.com changed reviewers: + chenwilliam@chromium.org, luoe@chromium.org
PTAL. This is my first patch, please run try jobs for me.
The CQ bit was checked by lushnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: The author Anton.Ulianov@gmail.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:851: var plainString = String.format(format, parameters, plainFormatters, '', (a, b) => a + b); Some of these formatters format objects, so you are appending objects to strings. Do you want to mute 'o' and 'O' ones as well? https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:853: var linkNode = Components.Linkifier.getLinkNode(plainString.formattedResult); What if we format("%s%s %s%s", "http", "://www.fb.com", "http", "://google.com"), how would it be linkified? https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:855: linkNode.innerHTML = ''; Use .textContent = instead, we never use innerHTML due to potential code injection. Also, you are destroying it first and then you are appending it into output? How does that work? https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:865: linkifyInner ? Components.linkifyStringAsFragment(String(b)) : createTextNode(String(b).trimMiddle(150)); That sounds like a strange heuristic. What you should do instead is when you get the formatted string in a node in line 876, you want to traverse the text nodes in that node and linkify them.
I think a version of Components.linkifyStringAsFragmentWithCustomLinkifier() that works on elements with TextNodes would be great. There wouldn't need to be any plainFormatter logic, and we could reuse your Components.linkifyElementAs...() in other places! To try out @pfeldman's suggested approach, you may want to check out traverseNextTextNode(). It's defined on the Node.prototype in the file DOMExtensions.js: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector/console/console-format-linkify.html (right): https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector/console/console-format-linkify.html:9: console.log("%cwww.%s", "color:lightgray", "google.com", 12) This case passes, but a similar case does not: console.log("%cwww.%s 12", "color:lightgray", "google.com") Can we have a plain test case here too? console.log("%cwww.google.com", "color: blue"); https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:636: * @return {?Array|{index:number, input:string}} This annotation looks like an Array OR an Object, but it seems like exec() returns just a nullable Array. It looks like we only ever use the [0] item in the array, if it exists. Maybe we can have this function return linkStringRegEx.exec(string)[0] or pathLineRegex.exec(string)[0] if it can? Then the closure return annotation can just be ?string
On 2016/12/13 at 00:08:27, luoe wrote: > I think a version of Components.linkifyStringAsFragmentWithCustomLinkifier() that works on elements with TextNodes would be great. There wouldn't need to be any plainFormatter logic, and we could reuse your Components.linkifyElementAs...() in other places! > > To try out @pfeldman's suggested approach, you may want to check out traverseNextTextNode(). It's defined on the Node.prototype in the file DOMExtensions.js: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... > I am not sure I've got the point here. We can't just linkkify TextNodes, because formatted string can be complex set of Nodes, which together looks like an url and should be linkified. for example, format('www.%cgoogle.com','color:blue') will result in such structure - TextNode(www) - span(style='color:blue') - TextNode(.google.com) And no text node should be linkified (and will not, since does not fit to regexp), but whole structure should be wrapped into link node. If you are suggesting to traverse all text nodes and concatenate them into the string, what is benefit here compared with getting same string in plainString? also, in case of format('www.%s 12', 'google.com') it will be more complex, since we need to linkify first two text nodes in one link concatenated, but not last one could you please elaborate?
Comments added, answers wanted. https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector/console/console-format-linkify.html (right): https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector/console/console-format-linkify.html:9: console.log("%cwww.%s", "color:lightgray", "google.com", 12) On 2016/12/13 at 00:08:27, luoe wrote: > This case passes, but a similar case does not: > console.log("%cwww.%s 12", "color:lightgray", "google.com") Yes, this is same case as @pfeldman noted, currently parts of string does not get linkified if there are substitutions in that parts. I'll figure out how to do that. > Can we have a plain test case here too? > console.log("%cwww.google.com", "color: blue"); sure https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:851: var plainString = String.format(format, parameters, plainFormatters, '', (a, b) => a + b); On 2016/12/12 at 22:05:45, pfeldman wrote: > Some of these formatters format objects, so you are appending objects to strings. Do you want to mute 'o' and 'O' ones as well? I would like to have output linkified only if it looks like url for user in console. If I mute objects, then console.log('www.%ogoogle.com') will looks like 'wwwObject.google.com', and will be linkified as 'www.google.com' and it will interfere with object's node which is also clickable. If we append objects to strings it will be converted to Node and then to string representation of it which does not fit url regexp and properly declined as url Perhaps it is better to consider string as non-url at all if it contains '%o' or '%O' substitutions to make that logic clear and more stable? https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:853: var linkNode = Components.Linkifier.getLinkNode(plainString.formattedResult); On 2016/12/12 at 22:05:45, pfeldman wrote: > What if we > > format("%s%s %s%s", "http", "://www.fb.com", "http", "://google.com"), > > how would it be linkified? This is a good point, I didn't take into account such case. Right now it will be linkified same way as it was before. Will add handling of such strings as well, but it will require more complex logic. Perhaps it will solve other concerns below in comments as well. https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:855: linkNode.innerHTML = ''; On 2016/12/12 at 22:05:45, pfeldman wrote: > Use .textContent = instead, we never use innerHTML due to potential code injection. Also, you are destroying it first and then you are appending it into output? How does that work? Ok. I'm using that node as accumulator for formatted string instead of wrapping formatted string into link node to avoid additional <span> element. So, first I get proper node for a message, and then build message inside that node. Since Linfifier._createLink creates link node and fills it's content with text url, I just remove that and replace with proper formatted message https://codereview.chromium.org/2568983003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:865: linkifyInner ? Components.linkifyStringAsFragment(String(b)) : createTextNode(String(b).trimMiddle(150)); On 2016/12/12 at 22:05:45, pfeldman wrote: > That sounds like a strange heuristic. > > What you should do instead is when you get the formatted string in a node in line 876, you want to traverse the text nodes in that node and linkify them. not sure I understand. in line 876 I will get same string or same string wrapped in style span, or another node which does not have plain text representation in console at all which could contain a link (like Object expandable node) If it is a Node which came in b parameter - no need to linkify that at all, if it is not node, then it converted to string, what is the point of putting that string into text node, and then extract back to linkify? If you referring to linkifyInner - I dont see how we can avoid that check. It is used to block links inside links in case like format('www.google.com:%d', 80)
Sure, I would like to see the complex cases mentioned in the comments handled, if possible. Currently there is no way to linkify TextNodes, as you pointed out. The plain string includes '%c''s which need to be substituted before we run a Regex, but applying substitution creates complex nodes with styles. Maybe there is a way to traverse through styled text nodes and group them? Or perhaps there is another way that is less complicated?
Implemented different approach to linkify and coloring, it covers more complex cases and result is more sane from user's point of view
Hi there, sorry, I was not looking at it for some and it seems to go sideways. What I meant and I think you should do is the following: 1) Build a non-linkified node A first (remove linkifier call from _formatWithSubstitutionString) 2) Extract function from Components.linkifyStringAsFragmentWithCustomLinkifier that returns matches + URLs as array or pairs: In: "Hellow, http://nyt.com". Out: [{text: "Hello "}, {text: "http://nyt.com", url: "http://nyt.com"}, ... ] 3) Get the T = A.textContent from the result node and linkify it using the function you extracted. 4) Patch node A using UI.highlightRangesWithStyleClass that creates spans for linkified regions, while preserving the %c style, etc. Your patch should add around ~50 lines of code.
Hi, I did look at the logic of UI.highlightRangesWithStyleClass, and not sure
how it can help here.
Although it looks like it's functionality is similar to required, what it does
it traverse all text nodes and concatenates them into new one, which then put
next to first text node in range.
After that any applied styles will be messed up, and DOM structure also looks a
little bit messy.
For example:
console.log('%cwww.google%c.com', 'color: gray', 'color:green')
gives that structure:
<span class="console-message-text">
<span style="color: lightgray;">www.google</span>
<span style="color: green;">.com</span>
</span>
Range to be linkified here is [offset: 0, length: 14], related to that match
[{text: 'www.google.com', url: 'www.google.com'}]
If I apply UI.highlightRangesWithStyleClass to 'console-message-text' node, it
will move all text inside 'lightgray' span, leaving 'green' empty. Resulting in
that structure:
<span class="console-message-text">
<span style="color: lightgray;">
""
<span class="some-highlight-class">www.google.com</span>
</span>
<span style="color: green;"></span>
</span>
Also, it does not help with creating 'link' span (I can patch that, providing
custom span creation function as a parameter, but not sure it is right way to
do)
Or did you mean I should patch node T with styles instead of patching A with
links?
So I should create unstyled linkified version of message first and then use
UI.highlightRangesWithStyleClass to apply styles on it's content, passing style
ranges instead of link ranges and patch that function to apply style instead of
class?
I think I can go that way, I'll need to calculate colored ranges, and split them
by links boundaries, and there is also 'o' and '_' to be handled properly, but
perhaps it still be not much of code.
So, can you please elaborate on 4), may be I am missing something here?
If UI.highlightRangesWithStyleClass results in what you are saying, we should fix it. It would be handy to have it not destroying the DOM.
Not sure if submitting a patch is enough to get all notified, so, adding a comment. Implemented with UI.highlightRangesWithStyleClass you've suggested, after fixing that function to be non destructive and more flexible, the rest was straightforward indeed. Sorry for that took so long, wasn't able to finish it for some time.
https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:608: Components.Linkifier.createLinkNodeWithCustomLinkifier = function(url, text, linkifier) { It looks like this method is doing nothing. Linkifier should be capable of splitting line and column itself. https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:625: Components.Linkifier.createLinkNode = function(url, text) { This method also does not do much. https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:643: Components.Linkifier.getLinkRegExp = function() { Does this need to be public? Start method with _ if not. https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:655: Components.Linkifier.getLinkRanges = function(string) { Do we need this public? https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:806: var linkRanges = Components.Linkifier.getLinkRanges(plainText); Instead of exposing a handful methods on Components.Linkifier, we could have one that linkifies by means of UI.highlightRanges. You should just say: Components.linkifyURLsInNode(formattedResult); https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:808: function linkifier(text, parent, idx) { You should annotate these parameters using jsdoc https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:811: if (parent && parent.style.cssText) Why is this necessary? That looks like something UI.highlightRanges should take care of. https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:879: * @return {!Array.<!Array.<!Element>>} Hm. That makes things quite complex. I would expect search to have higher priority than the link and override its style with the actual search highlight, while still producing single node as a result. It seems like we need to manage highlighters in a way that they are aware of each other and can have different priorities. Otherwise search result's rounded border would be split at the index url linkifier decides to split the node.
https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:608: Components.Linkifier.createLinkNodeWithCustomLinkifier = function(url, text, linkifier) { On 2017/01/27 at 19:10:07, pfeldman wrote: > It looks like this method is doing nothing. Linkifier should be capable of splitting line and column itself. It does not, this method called with three different linkifiers. From Linkifier itself, from ConsoleViewMessage._buildMessage and from TerminalWidget._linkifyTerminalLine. If i should make all of them capable of splitting line and column, it will be copypasted logic. I can move that logic to another place but it still be as helper method, used in Linkifier.linkifyScriptLocation and in createLinkNode(or linkifyURLsInNode should it be replaced). Perhaps it will result in less code after all, will try. https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:806: var linkRanges = Components.Linkifier.getLinkRanges(plainText); On 2017/01/27 at 19:10:07, pfeldman wrote: > Instead of exposing a handful methods on Components.Linkifier, we could have one that linkifies by means of UI.highlightRanges. You should just say: > > Components.linkifyURLsInNode(formattedResult); How it should handle the fact what ConsoleViewMessage requires some special style handling (see comment below)? https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:811: if (parent && parent.style.cssText) On 2017/01/27 at 19:10:07, pfeldman wrote: > Why is this necessary? That looks like something UI.highlightRanges should take care of. I think this logic is specific for ConsoleViewMessage, and in particular to _formatWithSubstitutionString, since it is only place in app where we want to transfer style attr from parent to highlighted prop. That logic and dom structure is internal for _formatWithSubstitutionString, it is created in local append function. In another places where UI.highlightRangesWithStyleClass used or could be used we should not do that style transfer (parent node could have some styles we don't want to duplicate on highlighted text, for example, padding or borders, or anything else). https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/2568983003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:879: * @return {!Array.<!Array.<!Element>>} On 2017/01/27 at 19:10:07, pfeldman wrote: > Hm. That makes things quite complex. I would expect search to have higher priority than the link and override its style with the actual search highlight, while still producing single node as a result. It seems like we need to manage highlighters in a way that they are aware of each other and can have different priorities. Otherwise search result's rounded border would be split at the index url linkifier decides to split the node. Yes, something looks wrong for me here as well, but I think it is because here we have two opposite requirements - destroy dom and create one text node for search, and keep dom structure and create set of text nodes for link. Do you mean highlightRangesWithStyleClass should be aware of that difference and produce single node or set of nodes depending on is it used for search or link? Problem with round borders exists, indeed, right now it solved on css level, and looks the same as before. I've found that more simple solution than put conditional logic for different use cases into highlightRangesWithStyleClass. Dont you think we should keep that way in sake of simplicity and keeping highlightRangesWithStyleClass more generic?
> It does not, this method called with three different linkifiers. From Linkifier > itself, from ConsoleViewMessage._buildMessage and from > TerminalWidget._linkifyTerminalLine. If i should make all of them capable of > splitting line and column, it will be copypasted logic. Copy-pasting 3 lines (with the use of helper method) is better than an ambiguous linkifier API - it is already too hairy. > I think this logic is specific for ConsoleViewMessage, and in particular to > _formatWithSubstitutionString, since it is only place in app where we want to > transfer style attr from parent to highlighted prop. I guess I'm missing the use-case because I don't get why we should clone the style. > Yes, something looks wrong for me here as well, but I think it is because here > we have two opposite requirements - destroy dom and create one text node for > search, and keep dom structure and create set of text nodes for link. > Do you mean highlightRangesWithStyleClass should be aware of that difference and > produce single node or set of nodes depending on is it used for search or link? The problem you are solving is considered a polish but, P3. It is important to fix and please our users, but if it comes at a cost of bloat in complexity, we have to pass. In this case, complexity is going up the order of magnitude (arrays of arrays), while the use case is only solved partially (search nodes are slit), hence the hesitation. Investing into a separable machinery (highlighter) that solves this kind of problems altogether on a generic level would justify the change. Otherwise, adding a bit of complexity into the code that is already not great, while fixing something of not the highest priority and not all the way, wouldn't be a great deal.
Description was changed from ========== Add ability to linkify substituted string Currently, if string contains substitutions, only separate parts of the string are replaced with links. This patch change logic to linkify whole string despite is it generated by substitutions or not BUG=668591 ========== to ========== Add ability to linkify substituted string Currently, if string contains substitutions, only separate parts of the string are replaced with links. This patch change logic to linkify whole string despite is it generated by substitutions or not BUG=668591 ==========
pfeldman@chromium.org changed reviewers: - pfeldman@chromium.org |
