Textarea Field, Multiline Block

[acbart]

The basics

• I branched from develop
• My pull request is against develop
• My code follows the style guide

The details

Resolves

#30

Proposed Changes

• Creates TextArea field that descends from TextInput
• Creates a Multiline Text Block
• Provides generators for new multiline text block
• Add multiline_quote_ function to all languages to properly handle creating multiline strings.
• Creates Pilcrow text mixin

Reason for Changes

A number of projects (e.g., BlockPy) make use of multiline text blocks. Common purposes include long strings, comments, and raw code blocks. This pull request makes the textarea another possible field type.

Test Coverage

I added the block to the playground and ensured that the code could be generated for all the languages.

Tested on:

• Desktop Chrome

Additional Information

• I'm not sure I handled every possible multiline edge case for Dart, Lua, PHP. I'm not as practiced with those languages, and it seems like it'd be possible to circumvent the text replacing easily. But then again, I'm not sure how much effort fighting those cases is worth.
• I used the Wikimedia commons for the Pilcrow image. Let me know if this is acceptable - seems like they have a permissive license.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[acbart]

I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[BeksOmega]

Thank you for adding this field! Its been requested for a very long time so it's super exciting that it's finally getting added!

Disclaimer: I'm not actually on the blockly team, but I have been doing a fields refactor lately so I took a look. Here is the proto-documentation for those changes, it might be interesting to look over.

Overall it looks very good, there are just a few notes in addition to the code comments:

1. The field doesn't round-trip through the XML correctly yet.
   
   This just requires you write custom serialization functions. I'd recommend having an attribute tag for each line (depending on your answer to note fix: unit tests were using strict equality testing for numeric results; now check 15 decimals #2).
2. Is each line meant to be handled separately, like a list? Or is the field meant to represent a paragraph? If it is the former I might consider changing the field to have a multi-part value. If it is the latter, I might consider adding text wrapping instead of requiring linebreaks.
3. Please create an issue for adding unit tests & validator blocks for this field.

Note to Blockly Team: I only looked at the field stuff, not the other stuff.

[acbart]

Wow, thanks for the in-depth feedback! I'll try to get this all responded to this weekend.

For question 1, I see what you mean about the round-trip. Seems like an issue with a new line being added. I'm not quite sure what part of the XML hijinx are causing that, so using the multi-part value may be an expedient way to bypass the issue.

Looking at question 2, I did view it as a single paragraph. In particular, I use this field in BlockPy for multiline strings and raw code blocks, so new lines are fairly precise. Text wrapping wouldn't be my first choice, personally, but if that was more proper for Blockly I wouldn't be opposed.

Regarding question 3, should I wait till the PR is accepted to create the issue?

[BeksOmega]

Looking at question 2, I did view it as a single paragraph. In particular, I use this field in BlockPy for multiline strings and raw code blocks, so new lines are fairly precise. Text wrapping wouldn't be my first choice, personally, but if that was more proper for Blockly I wouldn't be opposed.

Ah ok I see what you're going for! I'm not sure what the most common use case for a multiline text field is, so I'll defer to the team's judgment on this one.

Regarding question 3, should I wait till the PR is accepted to create the issue?

Hmmm... Now that you mention it I think you're right to wait on it. Wouldn't want anyone to try and jump on it and get confused.

[acbart]

• I fixed an issue where zooming would mess up the scale of the textarea.
• I'm not 100% convinced I have the textarea properly aligned on the rendered textelement, but I think it's close enough to be effective.
• In RTL mode, the text is still left aligned. I'm not sure what the precise proper behavior should be.

[acbart]

Okay, I think those commits resolve the issues raised so far. Let me know if there's more before it can be accepted.

[amber-cd]

Re: text wrapping, would it be appropriate to have the field accept some type of "wrap text" parameter to allow for both use cases? I hate to ask you to do more work, particularly as a volunteer who implemented an oft-requested feature (which looks great, by the way), but it seems like having some type of configuration parameter in the block constructor which controls text wrapping might be worthwhile.

[acbart]

Re: text wrapping, would it be appropriate to have the field accept some type of "wrap text" parameter to allow for both use cases?

The parameter would be easy to add, but what specific functionality would it trigger? Instead of triggering ellipsis, it just wraps onto a new line?

[amber-cd]

More or less. My thought would be specifically for something like, say, a "send email" block where you can send an email with the text and don't want to manually add in line breaks except when making new paragraphs, but want to be able to see the full text of each paragraph.

[rachel-fenichel]

Thanks for this! Here's a first pass review, and I'll pull the branch down and play with it today or tomorrow to finish off my review.

• I prefer the name FieldMultilineText. It distinguishes it from FieldTextInput.
• Please remove the generated language files: git checkout -- msg/json/en.json msg/json/qqq.json msg/js/*
• Other comments inline.

[acbart]

Haven't done anything with the text-wrapping (perhaps a separate issue, just to move this along?), but I renamed the field to MultilineText and got rid of the generated language files (oops). I'll be interested in hearing your more detailed review.

[rachel-fenichel]

I agree that the XML question can be a separate issue, since it sounds like there are a few different use cases.

On the generators: multiline strings are ES6, so handle it by adding together strings (or using an array and join) instead. Otherwise we get into a case where we can run in a browser (IE) and generate code that can't be executed in that same browser.

[NeilFraser]

A patch file for fixing some style nits:
0001-Fix-some-style-guide-nits.txt

I'm not convinced that the XML format currently being is the best option. Can't we encode the string with the usual escape characters? I think we could make the XML for text and multiline text be compatible and interchangeable. The XML format is one of the few things we don't get to change our minds on, so getting it right is important.

[NeilFraser]

Formal suggestion for XML format.

The current text input field encodes the string "<HTML>" like this:
<field name="TEXT">&lt;HTML&gt;</field>

Ideally the multiline text input would use exactly the same format. The problem is line feed characters.

• Using an actual line feed in the XML works, but makes manipulating the XML (e.g. pretty-printing it) very fragile.
• Using the string '\n' is not right since the literal string '\n' currently encodes as '\n' in a text input field. Besides, '\n' is not an XML format.
• Using the string '
' would seem to have no issues.

Thus, this pathological string:

Hello world
<HTML>
Literal \n string
Literal &#10; string

would encode as:

<field name="TEXT">Hello world&#10;&lt;HTML&gt;&#10;Literal \n string&#10;Literal &amp;#10; string</field>

The best part is that the encoding can be added to FieldTextInput, rather than FieldMultilineText.

[NeilFraser]

The best part is that the encoding can be added to FieldTextInput, rather than FieldMultilineText.

Correction: Field.to/fromXml would be where I'd suggest adding the 
 encoder/decoder.

[acbart]

Seems simple enough - do you want me to add this modification to Field in this pull request? It'd basically just be:

fieldElement.textContent = this.getValue().replace(/\n/g, '
');

wouldn't it?

[acbart]

I added the proposed fixes, by my understanding. Let me know if you had a different vision. Does seem simpler and more convenient - but given that I just modified Field, someone more familiar with the impacts should definitely check things over.

[NeilFraser]

Your changes are exactly as I described. And they don't work. My fault. :)

The issue is that we never explicitly encode or decode <, >, and &. That's handled automatically by the browser when one sets or gets textContent. Thus when we set a string that contains '
', it automatically gets escaped to '
'. As a result the current version of this PR will take the user text 'foo
bar', encode it as <field name="TEXT">foo&amp;#10;bar</field>, and round trip it back as

foo
bar

An approach I've tested that works is to remove the 
 from both Blockly.Field.prototype.to/fromXml, and instead edit Blockly.Xml.domToText:

Blockly.Xml.domToText = function(dom) {
  return Blockly.Xml.utils.domToText(dom).replace(/\n/g, '
');
};

[acbart]

So, I forgot to run jsunit before I pushed, but as you can see that seems to have broken the build. The issues seems to be that we're now encoding all newlines with '
', including the ones that are outside of the strings such as the newlines that are apparently put between XML nodes. Is this desirable? I'd need to update a number of unit tests, it seems.

[RoboErikG]

Looks like it's two tests:
test_domToText
test_domToPrettyText

My recommendation (unless Neil has another idea) would be to fix test_domToText by doing the same substitution in the expected string and fix test_domToPrettyText by modifying Blockly.Xml.domToPrettyText to substituting newlines back into the blob so it's still pretty. ;)

[NeilFraser]

Well that's interesting. The playground's XML does not reflect this. Give me a chance to fully understand what's going on here.

BTW, this change has the side-effect of improving the XML for multi-line comments. Fortunately in a backwards-compatible manner.

[NeilFraser]

This was fun. I don't think this issue would ever occur outside of a unit test, since it requires uselessly round-tripping XML without ever using it in Blockly. Also worth noting all the variants of this XML are perfectly valid and will work fine. The current issue is purely cosmetic (though any difference will fail the unit test's equality operator).

Nevertheless, here's a replacement function that resolves this issue. The use of a completely unreadable regular expression does not fill me with joy, however, if it fails there are no runtime consequences.

/**
 * Converts a DOM structure into plain text.
 * Currently the text format is fairly ugly: all one line with no whitespace,
 * unless the DOM itself has whitespace built-in.
 * @param {!Element} dom A tree of XML elements.
 * @return {string} Text representation.
 */
Blockly.Xml.domToText = function(dom) {
  var text = Blockly.Xml.utils.domToText(dom);
  // Replace line breaks in text content with '
' to make them single line.
  // E.g. <foo>hello\nworld</foo> -> <foo>hello&#10;world</foo>
  // Do not replace line breaks between tags.
  // E.g. ...</foo>\n</bar> is unchanged.
  // Can't use global flag on regexp since backtracking is needed.
  var regexp = /(<[^/][^<]*>[^<]*)\n([^<]*<\/)/;
  var oldText;
  do {
    oldText = text;
    text = text.replace(regexp, '$1&#10;$2');
  } while (text != oldText);
  return text;
};

JSUnit tests pass, as does Mocha.

[acbart]

I had been concerned that this additional regexing would negatively impact performance, but surprisingly it doesn't seem to have much effect on the timing runs I ran. I'll get this in and repush.

[NeilFraser]

XML looks good to me.

[acbart]

Where do we go from here? Do you need anything else from me, or does the review process just continue?

[RoboErikG]

Whoops, sorry for the delay. I think this is mostly good to go (@rachel-fenichel to confirm), but we'll wait until late next week to merge as we're finalizing this quarter's release this.

[rachel-fenichel]

Yup, it looks solid. We'll cut the release candidate at the end of this week or start of next week, and merge this into develop after that cut.

It looks like there is one trivial merge conflict from Neil's recent XML changes; either you'll fix that or I will next week.

[samelhusseini]

Closing in favour of #2663. Thanks @acbart