Opened 10 years ago

Closed 10 years ago

#10653 closed defect (fixed)

[regression] InlineEditBox: error with special chars

Reported by: joshilay Owned by: Douglas Hays
Priority: high Milestone: 1.5
Component: Dijit Version: 1.4.0
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

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

Attachments (2)

test.html (406 bytes) - added by joshilay 10 years ago.
test.html
10653.patch (4.1 KB) - added by Douglas Hays 10 years ago.
add initial value filtering if renderAsHtml=false, and created additional automated tests

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by Adam Peller

Milestone: 1.4.1tbd
Priority: highnormal
severity: majornormal

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

Changed 10 years ago by joshilay

Attachment: test.html added

test.html

comment:2 in reply to:  1 Changed 10 years ago by 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

comment:3 Changed 10 years ago by bill

Component: GeneralDijit
Milestone: tbd1.4.1
Owner: changed from anonymous to Douglas Hays
Summary: Error with dojo 1.4 embedded fonts like &lt;[regression] InlineEditBox: error with special chars

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.

comment:4 Changed 10 years ago by Douglas Hays

Resolution: invalid
Status: newclosed

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".

comment:5 Changed 10 years ago by Adam Peller

Milestone: 1.4.1tbd

comment:6 Changed 10 years ago by joshilay

Resolution: invalid
Status: closedreopened

comment:7 Changed 10 years ago by 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.

comment:8 Changed 10 years ago by Douglas Hays

Resolution: invalid
Status: reopenedclosed

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>

comment:9 Changed 10 years ago by bill

Description: modified (diff)
Resolution: invalid
Status: closedreopened

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.

Changed 10 years ago by Douglas Hays

Attachment: 10653.patch added

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

comment:10 Changed 10 years ago by Douglas Hays

Milestone: tbd1.5

I attached a patch file for trunk. Needs verification.

comment:11 Changed 10 years ago by 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.

comment:12 Changed 10 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

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

comment:13 Changed 10 years ago by bill

Resolution: fixed
Status: closedreopened

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>.

comment:14 Changed 10 years ago by 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.

comment:15 Changed 10 years ago by 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.

comment:16 Changed 10 years ago by bill

Resolution: fixed
Status: reopenedclosed

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.

Note: See TracTickets for help on using tickets.