Fix text box failing to Linkify paragraph children
Review Request #10152 — Created Sept. 21, 2018 and submitted
When making a paragraph with multiple newline links in a text box and
hitting 'ok' only the first link would be linkified.This was tested and it was found that the paragraph children were not
handling siblings correctly such that if one is replaced it causes the
loop variable 'node' becomes stale when referencing next siblings.The best method to get around this was to use the fact that all the child
nodes are available to be iterated in order. By doing so we avoid dealing
with pointer references which when replaced were causing the issues.Co-authored-by: Stuart Caie stuart.caie@oracle.com
Ran JS tests.
Ran unit tests.Tested by inputting paragraphs of links in different amounts.
Tested by putting plain text above and below paragraphs of links inclusive
and exclusively with multiple links and singular links.
Description | From | Last Updated |
---|---|---|
Can you rewrite your summary to: 1) be at most 50 characters long 2) be written in the imperitive mood, … |
brennie | |
Your description contains your summary as its first line. Also your second line of your description isnt a complete sentence. … |
brennie | |
The correct format is: Co-authored-by: Stuart Caie <stuart.caie@oracle.com> on a single line and as the last line. |
brennie | |
You have a typo in the function namespace. "RB.LInkify...." instead of "RB.Linkify....". On that note, when referencing code like that, … |
chipx86 | |
The description is wrapping wayyy too early. Typically, ~72 characters is a good wrapping point here. A good description also … |
chipx86 | |
Youre still wrapping your description/testing done too early (60ch instead of 72). You only need to give credit once. |
brennie | |
nit: Your summary doesn't need a period. |
brennie | |
nit: you do not need to mention failing tests on release-3.0.x We generally format what you have in your testing … |
brennie | |
So the suggested fix works, but we can't use for ... of in our code (see https://www.notion.so/reviewboard/JavaScript-ES6-Guidelines-a5d6efb850d24ac295e1b8792cecdd25) Instead, we can … |
david | |
Blank line between these. |
brennie |
- Bugs:
-
- Added Files:
- Testing Done:
-
Tested on windows:
Google Chrome 69.0.3497.100 Firefox: 62.0 Microsoft Edge: 42.17134.1.0 Ran unit tests on js-tests: all passed.
~ Ran unit tests on python-tests: all passed except test_diffutils.py ~ Ran unit tests on python-tests: all passed except test_diffutils.py (Seems like a release3.0.x issue | ie: appears on clean release-3.0.x branch unit test)
-
-
So the suggested fix works, but we can't use
for ... of
in our code (see https://www.notion.so/reviewboard/JavaScript-ES6-Guidelines-a5d6efb850d24ac295e1b8792cecdd25)Instead, we can just iterate with an index:
for (let i = 0; i < el.childNodes.length; i++) { const node = el.childNodes[i]; if (node.nodeType === node.TEXT_NODE) { ... }
- Commit:
-
6310aef7fe22273dbfb02475cf8184ef388377c29490d346df7bbc2e0785e684655ed4cb83718c70
Checks run (2 succeeded)
- Testing Done:
-
Tested on windows:
Google Chrome 69.0.3497.100 Firefox: 62.0 Microsoft Edge: 42.17134.1.0 Ran unit tests on js-tests: all passed.
~ Ran unit tests on python-tests: all passed except test_diffutils.py (Seems like a release3.0.x issue | ie: appears on clean release-3.0.x branch unit test) ~ Ran unit tests on python-tests: all passed except ones that also failed on release-3.0.x branch. + + Tested by inputting paragraphs of links in different amounts.
+ Tested by putting plain text above and below paragraphs of links inclusive and exclusively with multiple links and singular links.
-
-
Can you rewrite your summary to:
1) be at most 50 characters long
2) be written in the imperitive mood, i.e., as if it were a command. If you substitute your summary into the following sentence, it should make sense:This patch will <summary>
e.g., "Fix RB.LinkifyUtils.linkifyChildren on all browsers" or similar.
If you want to credit
kyz
for their work you can add the following to your summary and we will include it as a git trailer when we land:Co-authored-by: Stuart Caie <stuart.caie@oracle.com>
-
Your description contains your summary as its first line. Also your second line of your description isnt a complete sentence.
Give our guide on writing good change descriptions a read and update your description based on that.
-
- Summary:
-
EasyFix: Tested and fixed RB.LinkifyUtils.linkifyChildren with recommended solution from kyz.Fix text box failing to Linkify paragraph children.
- Description:
-
~ EasyFix: Tested and fixed RB.LinkifyUtils.linkifyChildren with recommended solution from 'kyz'.
~ Switched from iterating by sibling pointers in the for loop to using a for each iteration. ~ When making a paragraph with multiple newline
~ links in a text box and hitting 'ok' only + the first link would be linkified. + + Fixed RB.LInkifyUtils.linkifyChildren utility
+ to iterate paragraph child nodes using numeric + index with ES6 formatting. + + Co-authored-by:
+ - Stuart Caie stuart.caie@oracle.com + - For finding and suggesting proper fix
-
-
You have a typo in the function namespace. "RB.LInkify...." instead of "RB.Linkify....".
On that note, when referencing code like that, you can put a backtick on either side to turn it into a literal.
-
The description is wrapping wayyy too early. Typically, ~72 characters is a good wrapping point here.
A good description also helps people to better understand what was wrong and why and how it was addressed without digging into the code. From yours, I can understand what the problem was, and then I get some technical details that make no sense unless I know this code already. What you can do to improve the description is to give a summary of what went wrong and why, without assuming any prior knowledge. Something like "The reason that only the first line was linked was <...>. By doing <...>, we can properly cover all lines," or something like that.
- Description:
-
~ When making a paragraph with multiple newline
~ links in a text box and hitting 'ok' only ~ When making a paragraph with multiple newline links in a text box
~ and hitting 'ok' only the first link would be linkified. - the first link would be linkified. ~ Fixed RB.LInkifyUtils.linkifyChildren utility
~ to iterate paragraph child nodes using numeric ~ index with ES6 formatting. ~ This was tested and it was found that the paragraph children
~ were not handling siblings correctly if one is replaced and ~ that the loop variable 'node' becomes stale when referencing + next siblings. + Credit: Stuart Caie + The best method to get around this was to use the fact that + all the child nodes are available to be iterated in order. + By doing so we avoid dealing with pointer references which + when replaced were causing the issues. ~ Co-authored-by:
~ Co-authored-by: Stuart Caie stuart.caie@oracle.com
- - Stuart Caie stuart.caie@oracle.com - - For finding and suggesting proper fix - Testing Done:
-
Tested on windows:
Google Chrome 69.0.3497.100 Firefox: 62.0 Microsoft Edge: 42.17134.1.0 Ran unit tests on js-tests: all passed.
~ Ran unit tests on python-tests: all passed except ones that also failed on release-3.0.x branch. ~ Ran unit tests on python-tests: all passed except ones that + also failed on release-3.0.x branch. Tested by inputting paragraphs of links in different amounts.
~ Tested by putting plain text above and below paragraphs of links inclusive and exclusively with multiple links and singular links. ~ Tested by putting plain text above and below paragraphs of + links inclusive and exclusively with multiple links and singular links.
- Commit:
-
9490d346df7bbc2e0785e684655ed4cb83718c70688863402e8a5a59a0119d4bc979435999b80b21
Checks run (2 succeeded)
- Description:
-
~ When making a paragraph with multiple newline links in a text box
~ and hitting 'ok' only the first link would be linkified. ~ When making a paragraph with multiple newline links in a text box and
~ hitting 'ok' only the first link would be linkified. ~ This was tested and it was found that the paragraph children
~ were not handling siblings correctly if one is replaced and ~ that the loop variable 'node' becomes stale when referencing ~ next siblings. ~ Credit: Stuart Caie ~ The best method to get around this was to use the fact that ~ all the child nodes are available to be iterated in order. ~ This was tested and it was found that the paragraph children were not
~ handling siblings correctly such that if one is replaced it causes the ~ loop variable 'node' becomes stale when referencing next siblings. ~ ~ The best method to get around this was to use the fact that all the child
~ nodes are available to be iterated in order. By doing so we avoid dealing ~ with pointer references which when replaced were causing the issues. - By doing so we avoid dealing with pointer references which - when replaced were causing the issues. Co-authored-by: Stuart Caie stuart.caie@oracle.com
- Testing Done:
-
~ Tested on windows:
~ Google Chrome 69.0.3497.100 ~ Firefox: 62.0 ~ Ran unit tests on js-tests: all tests passed.
~ Ran unit tests on python-tests: all passed except ones that also failed ~ on release-3.0.x branch. - Microsoft Edge: 42.17134.1.0 - - Ran unit tests on js-tests: all passed.
- Ran unit tests on python-tests: all passed except ones that - also failed on release-3.0.x branch. Tested by inputting paragraphs of links in different amounts.
~ Tested by putting plain text above and below paragraphs of ~ links inclusive and exclusively with multiple links and singular links. ~ Tested by putting plain text above and below paragraphs of links inclusive ~ and exclusively with multiple links and singular links.
- Summary:
-
Fix text box failing to Linkify paragraph children.Fix text box failing to Linkify paragraph children
- Testing Done:
-
~ Ran unit tests on js-tests: all tests passed.
~ Ran unit tests on python-tests: all passed except ones that also failed ~ Ran JS tests.
~ Ran unit tests. - on release-3.0.x branch. Tested by inputting paragraphs of links in different amounts.
Tested by putting plain text above and below paragraphs of links inclusive and exclusively with multiple links and singular links.