Use jQuery to build DOM elements instead of string concatenation

[iabw]

Took a swing at #406

All the tests are still passing.

This probably demands new tests that ensure it's properly encoding special characters.

Also, as this will frequently be MUCH slower than string concatenation, should these editors (or the top schema) have a 'trustedContent' flag or something similar? That would then use string concat instead?

It probably wouldn't ever matter. "This isn't the bottleneck you're looking for", etc.

[philfreo]

doesn't look like you're using this anywhere?

[iabw]

Yeah, the close was from the old li tag opener
On Aug 27, 2014 12:19 PM, "Phil Freo" [email protected] wrote:

In src/editors/checkboxes.js:

       self.id = originalId;

•               close = false;
  

•      close = false;
  

doesn't look like you're using this anywhere?

—
Reply to this email directly or view it on GitHub
https://github.com/powmedia/backbone-forms/pull/411/files#r16786363.

[philfreo]

👍, ya I wouldn't worry about the performance of this too much... there are still very fewer DOM operations happening and overall this shouldn't be the bottleneck.

But yea, some additional tests would be nice

[iabw]

Do you have any thoughts on what the tests should be trying to do? Anything besides inserting some scripts that try to throw and checking the text content of the elements?

[philfreo]

ya I think just inserting some html characters into the various places (optgroups, options, etc) and checking that .text() on those elements is the same text is the main thing

[philfreo]

Labels are another spot that's problematic:
http://jsfiddle.net/dW2Qu/543/ (first one is bold)

By default titles probably should not be considered HTML, to prevent XSS in UGC cases, etc. However I could see cases where some people would want to use HTML, so maybe we should add a title_html option that (if set) allows HTML. Otherwise it would use title set via .text().

Thoughts?

[iabw]

That sounds like it might be the best option.

The only other thing that springs to mind is to force the title to be a template that accepts a config object, where you'd force the individual key values to be escaped and then inject those escaped values wherever the template dictated (like the change this commit made to the Radio editor template)

But that sounds like a pretty awful level of complexity for what I imagine is the typical use-case.

[iabw]

I'm not sure how I feel about doing the Field label escaping this way. It only applies to the default Field template. Users would need to know to add this feature if they wrote their own field template. (Although, I think bbf doesn't allow custom field templates passed in right now? That might be in another pull request of mine soon).

Possible Alternatives

• We could encode the text before it's ever passed to the template as part of the 'createTitle' method
• We could have titleHTML be not a flag, but a place to put HTML and render both all the time-

<label for="<%= editorId %>">\
      <%- title %><%= titleHTML %>\
</label>\

It also adds some whitespace around the label, which is annoying but a super long line is a style issue.

[philfreo]

We could have titleHTML be not a flag, but a place to put HTML and render both all the time
It also adds some whitespace around the label, which is annoying but a super long line is a style issue.

Seems like a good idea to me since it accomplishes everything in the simplest way. But adding one extra tag printout of an empty value shouldn't cause any extra whitespace... why is it?

[iabw]

Re: whitespace
I misspoke. I meant some space inside the 'label' tag around the text content, because of the template going onto multiple lines.

Re: Syntax
If the same value goes into title or titleHTML, it's functionally almost identical to the existing code. And if you prefer the semantics of the single key, it seems fine to me.

Are you saying you support the idea of having both title and titleHTML rendered all the time? Even though I put it out there, it seems kind of weird.

[philfreo]

Are you saying you support the idea of having both title and titleHTML rendered all the time?

I personally am fine with it - I've done this before and there's nothing weird about it unless someone sets both title and titleHTML at the same time, which means they're asking for trouble IMO :)

@powmedia do you have any thoughts on this?

[iabw]

Looking at it now, it seems like maybe Radio and Checkbox labels should be
treated however Field titles are. They should have both label and labelHTML
attributes?
On Aug 28, 2014 1:36 PM, "Phil Freo" [email protected] wrote:

Are you saying you support the idea of having both title and titleHTML
rendered all the time?

I personally am fine with it - I've done this before and there's nothing
weird about it unless someone sets both title and titleHTML at the same
time, which means they're asking for trouble IMO :)

@powmedia https://github.com/powmedia do you have any thoughts on this?

—
Reply to this email directly or view it on GitHub
#411 (comment)
.

[philfreo]

Yep, seems so

[powmedia]

Merged, thanks!