Added maxLines config option to MultilineInput #4477

gagiopapinni · 2020-11-27T21:30:03Z

The basics

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

The details

Resolves

#3969

Proposed Changes

Added a maxLines config option to multiline text input

Reason for Changes

#3969 proposed to add a scrolling functionality, to make working with large amounts of text more comfortable.
So one of the reasons is to close the issue, another - to make working with large amounts of text more comfortable :)

Test Coverage

Tested on:

Desktop Chrome
Desktop Firefox

Additional Information

Additionally, I have added a clamsy block to playground here to test it, so you can also play with it

google-cla · 2020-11-27T21:30:09Z

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 with @googlebot 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.

gagiopapinni · 2020-11-27T21:45:26Z

@googlebot I signed it!

maribethb

Thanks for the contribution! I left a few comments and questions.

maribethb · 2020-12-02T00:17:44Z

core/field_multilineinput.js

+   * If exceeded, scrolling functionality is enabled.
+   * @type {number}
+   */
+  this.maxLines_ = Infinity;


The trailing underscore should only be used for private/protected properties. In this case, since you already have public getters and setters, you should mark this property as protected.

Got it, will fix!

maribethb · 2020-12-02T00:23:45Z

core/field_multilineinput.js

    var text = lines[i];
-    if (text.length > this.maxDisplayLength) {
+    if (text.length > this.maxDisplayLength || overflow && i === displayLinesNumber - 1) {


It's correct either way, but could you add parentheses around the condition you added here? That way a reader doesn't have to stop and think about the order of operations like I did when I got here.

maribethb · 2020-12-02T00:56:46Z

core/field_multilineinput.js

@@ -211,6 +228,27 @@ Blockly.FieldMultilineInput.prototype.updateSize_ = function() {
    totalHeight += this.getConstants().FIELD_TEXT_HEIGHT +
        (i > 0 ? this.getConstants().FIELD_BORDER_RECT_Y_PADDING : 0);
  }
+  if (this.isBeingEdited_) {


Could you tell me a little more about what this section of code is doing? I'm not sure what you're trying to solve that isn't covered by the existing code (I checked out your PR and removed this bit and it seems fine, but I could be missing something). One difference I noticed is that when you have a truncated line (referring to maxDisplayLength, not maxLines), the block gets slightly wider when you start editing, then shrinks again when you finish. In the existing behavior, the block stays the same width. Unless we have good reason, I think we should prefer the existing behavior here.

Then, if the size doesn't change, I don't think you need to override showEditor_ either.

As you type in the editor, the underlying SVG text changes, and the width of the block is originally set as the longest SVG text element (above this section).
When we restrict the number of lines, then the block width is always restricted by the first maxLines number of SVG lines.
This results in a behavior where if maxLines is exceeded in editor mode, editor size no longer matches the longest line in the editor.
So here I set the block width as the longest string in editor, if it is being edited. This is important, otherwise you will have situations where the first maxLines lines have length of say 1, and then if another maxLines+1-th line followed with more characters, it would keep breaking down into tiny peaces, as shown below.

here's what it looks like without this section. The last line should not break but should resize the editor.

With this section enabled:

Also, the difference you were talking about here, that you had noticed, seems to be it was a bug, I think it's fixed now. That was happening because I was scaling the fontsize here before calculating line width. Actually, I still don't know why, but it seems to work perfectly fine without that, and even the bug is fixed :)

Thanks, makes sense, and glad you were able to fix the resizing issue! Could you add a comment summarizing what you said here? Something like "The default width is based on the longest line in the display text, but we need to calculate based on the absolute longest line even if it would be truncated after editing."

maribethb · 2020-12-02T01:02:07Z

core/field_multilineinput.js

@@ -286,9 +359,14 @@ Blockly.Css.register([
  '.blocklyHtmlTextAreaInput {',
    'font-family: monospace;',
    'resize: none;',
-    'overflow: hidden;',
+    'overflow: scroll;',
+    '-ms-overflow-style: none;',


The default should show the scrollbars since that is the typical paradigm for a scrollable text area. You can remove them for your project in your own css if you really don't want to show them.

I will fix this too in a few days.

I cant just enable them because it will result in improper block size, and text will break down in places where it should not. Last time I tried updating block size in a way to include scrollbar width by using clientWidth, got some strange glitches, so I need a little more time to figure this one out.

gagiopapinni · 2020-12-08T18:52:59Z

core/field_multilineinput.js

+  if (this.isBeingEdited_) {
+    var htmlInput = /** @type {!HTMLElement} */(this.htmlInput_);
+    if (this.isOverflowedY_) {
+      Blockly.utils.dom.addClass(htmlInput, 'blocklyHtmlTextAreaInputOverflowedY');
+    } else {
+      Blockly.utils.dom.removeClass(htmlInput, 'blocklyHtmlTextAreaInputOverflowedY');
+    }
+  }
+


So I should probably explain why I have decided enabling the scrollbar manually like this.

Initially I tried using overflow-y:auto;, but because the textarea is not resizable, I would get a glitch like this:

As I understand it, when we have a non-resizable textarea filled with text, another symbol automatically triggers scrollbar, and even though the next moment we set the correct width, the scrollbars' width is still there and it still counts as existing. Probably has something to do with the reflow order... and so we get the phantom scrollbar space. I tried fixing this thing, but eventually it got too painful, and I settled with current approach.

maribethb

Looks good, just requesting that you add some of the explanations you gave here into the code so it's clear for future readers.

Please note that as we have the release next week, develop is currently frozen except for bug fixes so that we have adequate time to test all feature additions. This means after approval, your PR won't be merged for a few days until after the release (and possibly a bit longer than that since most of us will be taking vacation through the end of December) and your feature will be included in the following (q1 2021) release of Blockly. Thanks again for your work on this!

maribethb · 2020-12-10T00:27:50Z

core/field_multilineinput.js

@@ -211,6 +228,27 @@ Blockly.FieldMultilineInput.prototype.updateSize_ = function() {
    totalHeight += this.getConstants().FIELD_TEXT_HEIGHT +
        (i > 0 ? this.getConstants().FIELD_BORDER_RECT_Y_PADDING : 0);
  }
+  if (this.isBeingEdited_) {


Thanks, makes sense, and glad you were able to fix the resizing issue! Could you add a comment summarizing what you said here? Something like "The default width is based on the longest line in the display text, but we need to calculate based on the absolute longest line even if it would be truncated after editing."

maribethb · 2021-01-07T21:55:23Z

Thanks for your contribution! And your patience, as develop was frozen for release and then most of us were on holiday. :) This should be available in the next release.

LarryKlugerDS · 2021-04-01T14:22:44Z

This is a great PR! Thank you so much @gagiopapinni !

Added maxLines config option

d8cbc7b

gagiopapinni closed this Nov 27, 2020

gagiopapinni reopened this Nov 27, 2020

rachel-fenichel requested a review from maribethb December 1, 2020 21:40

maribethb reviewed Dec 2, 2020

View reviewed changes

gagiopapinni added 5 commits December 4, 2020 13:14

Made maxLines_ protected

04dc6b6

Added parentheses

c1beab7

fontsize fix

d3ce839

Enabled scrollbars at overflow of y axis

25d4826

Added ellipsis handling for overflow Y

8cfd161

gagiopapinni commented Dec 8, 2020

View reviewed changes

Deleted unused variable

aaef7db

gagiopapinni requested a review from maribethb December 9, 2020 15:02

maribethb approved these changes Dec 10, 2020

View reviewed changes

Added an explanation comment

55a3476

maribethb merged commit e4decd8 into google:develop Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added maxLines config option to MultilineInput #4477

Added maxLines config option to MultilineInput #4477

gagiopapinni commented Nov 27, 2020 •

edited

google-cla bot commented Nov 27, 2020

gagiopapinni commented Nov 27, 2020

maribethb left a comment

maribethb Dec 2, 2020

gagiopapinni Dec 2, 2020

maribethb Dec 2, 2020

gagiopapinni Dec 2, 2020

maribethb Dec 2, 2020

gagiopapinni Dec 2, 2020

gagiopapinni Dec 9, 2020

maribethb Dec 10, 2020

maribethb Dec 2, 2020

gagiopapinni Dec 2, 2020 •

edited

gagiopapinni Dec 8, 2020 •

edited

maribethb left a comment

maribethb Dec 10, 2020

maribethb commented Jan 7, 2021

LarryKlugerDS commented Apr 1, 2021

Added maxLines config option to MultilineInput #4477

Added maxLines config option to MultilineInput #4477

Conversation

gagiopapinni commented Nov 27, 2020 • edited

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Additional Information

google-cla bot commented Nov 27, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

gagiopapinni commented Nov 27, 2020

maribethb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagiopapinni Dec 2, 2020 • edited

Choose a reason for hiding this comment

gagiopapinni Dec 8, 2020 • edited

Choose a reason for hiding this comment

maribethb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribethb commented Jan 7, 2021

LarryKlugerDS commented Apr 1, 2021

gagiopapinni commented Nov 27, 2020 •

edited

gagiopapinni Dec 2, 2020 •

edited

gagiopapinni Dec 8, 2020 •

edited