[regression] InlineEditBox: error with special chars

[joshilay]

With dojo 1.4, InlineEditBox with an initial value containing special characters like < > & is not being displayed correctly. Instead page is showing &lt; etc.

[Adam Peller]

You'll need to give a little more detail, preferably with an example. Which code is not processing entities like &lt; ?

[joshilay]

test.html

[joshilay]

Replying to peller:

You'll need to give a little more detail, preferably with an example. Which code is not processing entities like &lt; ?

I added a test case which shows with inline edit box embedded fonts are not working but outside it embedded fonts are working . I am using FF 3.5.7

[bill]

Thanks for attaching the test case. One key thing that you left out of your original report is that you were talking about dijit, and specifically about InlineEditBox. :-)

This worked in 1.3 and is broken in 1.4, starting with [20659]. Doug, can you look at this? Might be worth putting into 1.4.1.

[Douglas Hays]

In 1.3, the renderAsHtml flag defaulted to false but was being ignored. If you want the actual HTML entities to show on the page, then set renderAsHtml="true".

[joshilay]

By using renderAsHtml="true", inlineeditbox shows special characters like &lt; correctly but when user clicks on the inlineeditbox to edit text, it again shows "&lt;" instead of correct value.

[Douglas Hays]

It is behaving correctly. The user should be able to enter:

this is <b>&lt;bold&gt;</b>

and the HTML will be rendered as
this is <bold>

[bill]

This seems broken to me too.

renderAsHtml=true is inappropriate here, because that setting is for when the editor (ie: the widget shown when you click the InlineEditBox) handles HTML, which in practice means dijit.Editor. Since this test case is using the default dijit.TextBox editor, it should stick to the default renderAsHtml=false.

Saving in renderAsHtml=false mode is working correctly: the user can enter <foo> into the TextBox and when editing is finished it still shows up on the screen as <foo>.

However, the problem though is to specify an initial value of the InlineEditBox that contains special characters. How does a developer specify the initial value as <Inside Inline Edit Box>? I don't see any way. In any case, markup like this doesn't work in 1.4:

<span dojoType="dijit.InlineEditBox">
       &lt; Inside Inline Edit Box&gt;
</span>

That it worked in 1.3 and I think it makes sense.

[Douglas Hays]

add initial value filtering if renderAsHtml=false, and created additional automated tests

[Douglas Hays]

I attached a patch file for trunk. Needs verification.

[bill]

Looks like it will work, although it seems strange to have a this.attr('value', ...) call at creation, as the main thing it does is just setting innerHTML to itself. (Note that if the caller explicitly sets the value parameter, _setValueAttr() will be called by the widget infrastructure, so in that case too calling this.attr() from postMixInProperties() is superflous.) I think you could replace the code in postMixInProperties() with:

if(!this.value){
   this.value = dojo.trim(this.renderAsHtml ? this.displayNode.innerHTML :
      (this.displayNode.innerText||this.displayNode.textContent));
}
if(!this.value){
    this.displayNode.innerHTML = this.noValueIndicator;
}

I thought about reducing it further by having a value getter method (since there's no real reason to extract the value from innerHTML on creation), although it's complicated by that noValueIndicator:

_getValueAttr: function(){
      return this.displayNode.innerHTML =- this.noValueIndicator ? "" :
                 ( this.renderAsHtml ? this.displayNode.innerHTML :
                 (this.displayNode.innerText||this.displayNode.textContent) );
}

(Would need to change the edit() method to call attr("value") instead of accessing this.value directly.)

I'm worried about that breaking because the [this.displayNode.innerHTML == this.noValueIndicator test might return false just because of spurious differences like case, attribute order, <img> vs <img/> etc.

[Douglas Hays]

(In [21288]) Fixes #10653. InlineEditBox::postMixInProperties now changes HTML entities to single characters if renderAsHtml=false (and adds newlines for BR tags)

[bill]

This still seems broken to me, and actually robot/InlineEditBox.html is failing (in both [21288] and the latest on trunk), on at least FF and Chrome.

Regardless of the renderAsHTML setting, the initial DOM in the <div dojoType=dijit.InlineEditBox> node should not be modified by InlineEditBox widget until the user clicks to edit and then saves.

The test file is:

<h2>Complex renderAsHtml="false"</h2>
<div id="renderAsHtml_false" dojoType="dijit.InlineEditBox" renderAsHtml="false" width="400px" style="width:400px;">
	&lt;B&gt;not bold&lt;/B&gt;<B>not bold</B>&amp;lt;input&amp;gt;
</div>
<hr style="width:100%;">
<h2>Complex renderAsHtml="true"</h2>
<div id="renderAsHtml_true" dojoType="dijit.InlineEditBox" renderAsHtml="true" width="400px" style="width:400px;">
	&lt;B&gt;not bold&lt;/B&gt;<B>bold</B>&amp;lt;input&amp;gt;
</div>

That means that when the page is rendered it should appear as:

Complex renderAsHtml="false

<B>not bold</B>not bold&<input&>

---

Complex renderAsHtml="true"

<B>not bold</B>not bold&<input&>

In other words, on initial page load it should display the same regardless of whether the markup is <div dojoType=dijit.InlineEditBox> or just <div>.

[Douglas Hays]

I disagree. The renderAsHtml=false test with the initial bold text was intentionally added to make sure the widget was able to modify user text to correct an invalid situation. With renderAsHtml=false, the non-edit value of &lt;B&gt; is translated to <B> when editing since that's how's its rendered and thus the user is just changing text and not HTML. If a non-edit value of <B> is allowed, then it would not be discernable from the former &lt;B&gt;. The only way for this to work would be for the non-edit value of &lt;B&gt; to not be translated to <B> when editing, which would make editing confusing since the displayed characters do not match the editing characters which is a design-point for renderAsHtml=false.

[Douglas Hays]

(In [21390]) References #10653. Changed robot test to not use the left arrow to move the input caret to the start of a highlighted block since that's not a cross-browser way to move. The right arrow does seem to move to the end on all browsers so use that instead.

[bill]

I see... so I mispoke above, what I should have said was that:

<div dojoType="dijit.InlineEditBox" renderAsHtml="false" ...>
	<b>bold</b>
</div>

should be unsupported (leading to undefined behavior). I understand that you want to correct for developer errors like malformed HTML:

<div dojoType="dijit.InlineEditBox" renderAsHtml="false" ...>
	sticks & stones
</div>

which should have been:

<div dojoType="dijit.InlineEditBox" renderAsHtml="false" ...>
	sticks &amp; stones
</div>

However, I just don't want to add that overhead to the InlineEditBox code, in particular the performance hit of modifying the InlineEditBox.innerHTML on widget instantiation. It's possible the error above is a common error but I don't think it is, as it's no different than others times that "plain text" (with possible reserved characters) is inserted into a web page.

So, I'll mark this ticket as fixed since the reported issue (the last example above) is now working correctly, but I'm going to roll back the part about correcting malformed markup.