#11801 - Restricted Font Size Input To Only Valid Entries #13123

nyteksf · 2017-02-24T22:28:24Z

Related to #11801.

Text was able to be inputted into Font Size within Themes Settings (e.g. "View > Themes...") even when it included non-ASCII characters or was in a generally unusable format.

Corrected this issue by ensuring all font sizes are to begin with a number, and are completed by either "px" or "em" only. 'Done' button remains disabled until said format is adhered to correctly--at which time the functionality of said button becomes restored allowing input to be saved.

@zaggino

zaggino

just a first quick look without testing, let me know when you'll look at the comments

zaggino · 2017-02-24T22:43:54Z

src/config.json

@@ -20,7 +20,7 @@
        "extension_url": "https://s3.amazonaws.com/extend.brackets/{0}/{0}-{1}.zip",
        "linting.enabled_by_default": true,
        "build_timestamp": "",
-        "healthDataServerURL": "https://healthdev.brackets.io/healthDataLog"
+        "healthDataServerURL": "https://health.brackets.io/healthDataLog"


you need to revert this change (it applies automatically after doing npm install, but we don't want it in PRs)

zaggino · 2017-02-24T22:46:19Z

src/view/ThemeSettings.js

+                var $target = $(this);
+                var $input = $target.val();
+                var validInput = /^[\d\.]+(p|px|e|em){0,1}$/g;
+                var btn = document.getElementById("done");


add document to the globals please: https://github.com/adobe/brackets/blob/master/.eslintrc.json#L52

zaggino · 2017-02-24T22:47:46Z

src/htmlContent/themes-settings.html

                </div>
            </div>
        </form>
    </div>
    <div class="modal-footer">
        <button class="dialog-button btn" data-button-id="cancel">{{Strings.CANCEL}}</button>
-        <button class="dialog-button btn primary" data-button-id="save">{{Strings.DONE}}</button>
+        <button class="dialog-button btn primary" id="done" data-button-id="save">{{Strings.DONE}}</button>


id done is not very good, you should be more specific when introducing id's, something like theme-settings-done-btn would be more appropriate

zaggino · 2017-02-24T22:49:19Z

src/htmlContent/themes-settings.html

                </div>
            </div>

            <div class="control-group">
                <label class="control-label">{{Strings.FONT_FAMILY}}:</label>
                <div class="controls">
-                    <input type="text" data-target="fontFamily" value="{{settings.fontFamily}}">
+                    <input type="text" id="font-family-input" data-target="fontFamily" value="{{settings.fontFamily}}">


don't need to introduce id's here, you can use .on("input", "[data-target=fontFamily]", function () {
https://www.w3schools.com/jquery/sel_attribute_equal_value.asp

zaggino · 2017-02-24T22:50:58Z

src/view/ThemeSettings.js

-            .on("input", "[data-target]:text", function () {
+            .on("input", "#font-size-input", function () {
+                var $target = $(this);
+                var $input = $target.val();


$ prefix for variables indicates that it's a jquery (or a DOM) element
in this case $input is a string and should really be named input or even better targetValue

zaggino · 2017-02-24T22:52:57Z

src/view/ThemeSettings.js

+                // Make sure that the font size is expressed in terms we can handle (px or em). If not, 'done' button disabled until input corrected.
+
+                if ($input.search(validInput) === -1) {
+                    ($input.length === 0) ? btn.disabled=false : btn.disabled=true;


btn.disabled = $input.length !== 0 is better

zaggino · 2017-02-24T22:55:34Z

src/view/ThemeSettings.js

+                if ($input.search(validInput) === -1) {
+                    ($input.length === 0) ? btn.disabled=false : btn.disabled=true;
+                    return false;
+                }else{


}else{ after return doesn't make sense, just close the if with } and continue normally

zaggino · 2017-02-24T23:29:51Z

also make sure command grunt eslint in your branch doesn't throw any errors

ficristo · 2017-02-25T13:20:55Z

.eslintrc.json

@@ -57,6 +57,7 @@
        "$": false,

        "window": false,
+        "document": false,


Sorry for mixed signals, but please revert this.
document is a generic word and in Brackets usually refer to the Document object.
I think you can go with jquery here, can't you? $("#theme-settings-done-btn")

Sorry for the delay--I've been busy with moving, although wanted to finish and reply. That said,, yes, I can use jQuery to avoid going here. Good point. And thanks for clarifying. This is my first pull request; let alone being my first open source patch. I wasn't sure if I should use jQuery as little as possible as noobish as that sounds. This turned out to be a simple fix. :)

ficristo · 2017-02-25T13:21:45Z

Out of curiosity, any reason to not use the pattern attribute?

zaggino · 2017-02-26T04:33:11Z

Yeah, using pattern ( https://www.w3schools.com/tags/att_input_pattern.asp ) directly on the input would probably be better.

mohanad1980 · 2017-02-26T05:32:20Z

hello can you contact me [email protected]

…ibute, jQuery for var $btn, etc.

nyteksf · 2017-02-27T08:28:10Z

Thanks, guys, for your speedy replies. And the link. To reply now to @ficristo: that HTML5 pattern attribute was actually wholly new to me. I've learned and relearned a bunch of neat stuff here in just a few reviewer comments. And I have to agree with you both that "pattern" is much better in usage. Thanks again! Please feel free to let me know if anything else would be problematic.

zaggino · 2017-02-27T08:38:11Z

fine with me, @ficristo ?

ficristo · 2017-02-27T08:54:18Z

It seems fine for me.
Ideally we could expose this regex:

brackets/src/view/ViewCommandHandlers.js

Line 300 in d6ae906

validFont = /^[\d\.]+(px|em)$/;

and use it on the html.

zaggino · 2017-02-27T09:05:47Z

@nyteksf to do what @ficristo wrote, you'd have to edit this file https://github.com/nyteksf/brackets/blob/9a6f12cf259276ebd163e70266030377df67eb2b/src/view/ViewCommandHandlers.js and make this

brackets/src/view/ViewCommandHandlers.js

Line 300 in d6ae906

validFont = /^[\d\.]+(px|em)$/;

an exported variable.

Then here https://github.com/nyteksf/brackets/blob/9a6f12cf259276ebd163e70266030377df67eb2b/src/view/ThemeSettings.js#L82 you'd pass the ViewCommandHandlers.validFontRegex (or however you call it) to Mustache and render it here https://github.com/nyteksf/brackets/blob/9a6f12cf259276ebd163e70266030377df67eb2b/src/htmlContent/themes-settings.html#L33 like {{ fontPattern }} or something

Hope that's clear enough, @ficristo correct me if I'm wrong

petetnt · 2017-02-27T12:25:28Z

LGTM, but we should accept at least rem values and percentages too, no?

zaggino · 2017-02-27T12:32:38Z

Current regexp which is used deeper (

brackets/src/view/ViewCommandHandlers.js

Line 300 in d6ae906

validFont = /^[\d\.]+(px|em)$/;

) seems to only allow px|em ... do we want to change this?

nyteksf · 2017-03-03T01:35:27Z

Thank you, guys. I have had to fiddle around with the RegExp a little to make the prescribed changes work. It seems backslashes need to be escaped to be exported successfully with mustache--having tried the various remedies in the docs that seemed to apply--and this causes the RegExp constructor used for the non-exported segment to be passed something unintended in that case (e.g., \\ and d instead of \d). Likewise, to use RegExp literally, it seems would require .slice()ing off the foreslashes to be made compatible with the HTML5 Pattern Attribute for export thereunto.

That said, rather than attempting to escape any characters, I have switched the RegExp in question to "[0-9.]..." instead of "[\d.]". But everything else should be as asked for to my awareness. Please let me know what you think in any case. Thanks very much. :)

@zaggino @ficristo

zaggino

just a few small changes that are of stylistic nature and I believe we're good to merge this

zaggino · 2017-03-03T01:40:46Z

src/htmlContent/themes-settings.html

@@ -30,7 +30,7 @@ <h1 class="dialog-title">{{Strings.THEMES_SETTINGS}}</h1>
            <div class="control-group">
                <label class="control-label">{{Strings.FONT_SIZE}}:</label>
                <div class="controls">
-                    <input type="text" data-target="fontSize" value="{{settings.fontSize}}">
+                    <input type="text" pattern="{{ settings.validFontSizeRegExp}}" data-target="fontSize" value="{{settings.fontSize}}">


missing space before }}

zaggino · 2017-03-03T01:42:01Z

src/view/ThemeSettings.js

        return result;
    }
-
+    var validFontSizeRegExp = ViewCommandHandlers.validFontSizeRegExp;


constant definitions should always be on top of the file after imports, or you can use ViewCommandHandlers.validFontSizeRegExp directly

zaggino · 2017-03-03T01:42:42Z

src/view/ThemeSettings.js

@@ -81,7 +82,7 @@ define(function (require, exports, module) {
        var template        = $("<div>").append($settings).html();
        var $template       = $(Mustache.render(template, {"settings": currentSettings, "themes": themes, "Strings": Strings}));

-        // Select the correct theme.
+// Select the correct theme.


invalid whitespace change

zaggino · 2017-03-03T01:43:30Z

src/view/ViewCommandHandlers.js

-
-        // Make sure that the font size is expressed in terms we can handle (px or em). If not, simply bail.
-        if (fsStyle.search(validFont) === -1) {
+     var validFontSizeRegExpStr = "^[0-9.]+(px|em)$"; // Need RegExp string to be exported for compatibility with HTML5 pattern attribute.


constant definitions should always be on top (after import declarations) and not between functions, also needs a comment, something like:

/* * Font sizes should be validated by this regexp */

nyteksf · 2017-03-03T02:22:33Z

Whoops! Well, okay: there you go. Thanks again for a speedy reply!

@zaggino

zaggino · 2017-03-03T02:31:08Z

Testing this, it allows input like:
12.00.00px
can we please change regexp to be maybe:
"^[0-9]+(\\.[0-9]+)?(px|em)$"

zaggino · 2017-03-03T02:33:44Z

Done with testing, just change this one and I'm merging this (promise 😄 )

nyteksf · 2017-03-03T05:16:50Z

Okay. I'm going out on a limb here: so of course please do correct me if my suggestion happens to be wrong. I added your change to the original deeper RegExp. Then in doing my own testing, I had found that by comparison to the original it worked almost perfectly. That RegExp in question, however, didn't handle cases like ".5em", without the user first adding a zero to make it like "0.5em". That isn't a problem, I'm sure. My change seems like a small change indeed with that being said--but I figured I could change it still for sake of ease of use. And now it should handle cases like "0.5em" as well as ".5em". Please let me know if it works as it seems to on my end. And thanks again for all your help. :)))

@zaggino

nyteksf · 2017-03-03T05:28:42Z

Nevermind. I found a flaw with my last change. I am re-changing the RegExp to address cases like "14.0.0px", which still weren't prevented. My bad.

nyteksf · 2017-03-03T05:38:22Z

Okay, @zaggino. My fingers are crossed this time. I'm sending my final changes over your way if you'll please take another look now. :)

zaggino · 2017-03-03T06:25:37Z

Oh, github new features are superb, did a small change to your regexp through the web - when inside a string you need to escape \. like this \\. - otherwise an expression 12a5px would be valid, I've fixed that

zaggino · 2017-03-03T06:30:11Z

Looks good to me now. I think all comments above have been considered so I'm merging this.
Thanks a lot for the PR a for sticking with it to the end, hope we'll meet again on some other one.
Cheers!

nyteksf added 2 commits February 24, 2017 13:35

Restricted font size input to valid entries.

df77e15

#11801 - Restricted font size input to valid entries.

7689017

zaggino suggested changes Feb 24, 2017

View reviewed changes

nyteksf added 4 commits February 24, 2017 17:33

#11801 - Added @zaggino's prescribed changes

2143187

Deleted trailing spaces, and added missed change

f7a0cab

#11801 - Removed line of whitespace

01ae3a8

#11801 - Added inner quotes

8131ef4

ficristo reviewed Feb 25, 2017

View reviewed changes

nyteksf added 2 commits February 27, 2017 00:08

#11801 - Implemented @ficristo's Requested Changes: HTML5 pattern att…

337946f

…ibute, jQuery for var $btn, etc.

#11801 - Edited tag value for clarity

9a6f12c

zaggino approved these changes Feb 27, 2017

View reviewed changes

#11801 - Added @ficristo's Changes: Exposed RegExp To Be Used On HTML

a5253e7

zaggino suggested changes Mar 3, 2017

View reviewed changes

#11801 - Added @zaggino's Stylistic Changes

8839926

#11801 - Trimmed Edge Cases From Font Size Validating RegExp

fdfdcc5

#11801 - Corrected Format Of RegExp

febb9cb

slash should be escaped

e337dc7

this variable was never used

737f086

zaggino merged commit 2f5bf7e into adobe:master Mar 3, 2017

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] #11801 - Restricted Font Size Input To Only Valid Entries brackets-archive/bracketsIssues#11130

Open

#11801 - Restricted Font Size Input To Only Valid Entries #13123

#11801 - Restricted Font Size Input To Only Valid Entries #13123

Uh oh!

Conversation

nyteksf commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zaggino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaggino commented Feb 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nyteksf Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ficristo commented Feb 25, 2017

Uh oh!

zaggino commented Feb 26, 2017

Uh oh!

mohanad1980 commented Feb 26, 2017

Uh oh!

nyteksf commented Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zaggino commented Feb 27, 2017

Uh oh!

ficristo commented Feb 27, 2017

Uh oh!

zaggino commented Feb 27, 2017

Uh oh!

petetnt commented Feb 27, 2017

Uh oh!

zaggino commented Feb 27, 2017

Uh oh!

nyteksf commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zaggino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaggino Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nyteksf commented Mar 3, 2017

Uh oh!

zaggino commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zaggino commented Mar 3, 2017

Uh oh!

nyteksf commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nyteksf commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nyteksf commented Feb 24, 2017 •

edited

Loading

nyteksf Feb 27, 2017 •

edited

Loading

nyteksf commented Feb 27, 2017 •

edited

Loading

nyteksf commented Mar 3, 2017 •

edited

Loading

zaggino Mar 3, 2017 •

edited

Loading

zaggino commented Mar 3, 2017 •

edited

Loading

nyteksf commented Mar 3, 2017 •

edited

Loading

nyteksf commented Mar 3, 2017 •

edited

Loading