[Caja] Loosely parsing text "&#8224" as "†"

Adding functionality in caja to be able to do more lenient parsing and converting text "&#8224" to "†".

[anupama.dutta, 2010-08-09 11:14:47]

http://codereview.appspot.com/1866052/diff/2001/3001
File tests/com/google/caja/parser/html/Html5ElementStackTest.java (right):

http://codereview.appspot.com/1866052/diff/2001/3001#newcode59
tests/com/google/caja/parser/html/Html5ElementStackTest.java:59: "Hello &#8224
World", HtmlTokenType.TEXT, FilePosition.UNKNOWN);
Adding another text snippet with † and showing that it will be retained as
before would be good.

[gagan.goku, 2010-08-09 12:11:30]

http://codereview.appspot.com/1866052/diff/2001/3001
File tests/com/google/caja/parser/html/Html5ElementStackTest.java (right):

http://codereview.appspot.com/1866052/diff/2001/3001#newcode59
tests/com/google/caja/parser/html/Html5ElementStackTest.java:59: "Hello &#8224
World", HtmlTokenType.TEXT, FilePosition.UNKNOWN);
On 2010/08/09 11:14:47, anupama.dutta wrote:
> Adding another text snippet with † and showing that it will be retained
as
> before would be good.

Added another unit test case below.

[Kuntal Loya, 2010-08-09 19:12:38]

http://codereview.appspot.com/1866052/diff/9001/10002
File src/com/google/caja/parser/html/Html5ElementStack.java (right):

http://codereview.appspot.com/1866052/diff/9001/10002#newcode523
src/com/google/caja/parser/html/Html5ElementStack.java:523: return
text.replaceAll("(\\b|\\B)(&#\\d{2,4});?", "$2;");
Is \b|\B required?

Btw, there can be any number of digits (dont know the upper limit) after &#,
which if have a ; missing at the end, would still be a valid escape character.

[MikeSamuel, 2010-08-09 19:34:49]

http://codereview.appspot.com/1866052/diff/9001/10002
File src/com/google/caja/parser/html/Html5ElementStack.java (right):

http://codereview.appspot.com/1866052/diff/9001/10002#newcode523
src/com/google/caja/parser/html/Html5ElementStack.java:523: return
text.replaceAll("(\\b|\\B)(&#\\d{2,4});?", "$2;");
On 2010/08/09 19:12:38, Kuntal Loya wrote:
> Is \b|\B required?
> 
> Btw, there can be any number of digits (dont know the upper limit) after &#,
> which if have a ; missing at the end, would still be a valid escape character.

To answer Kuntal's question, the value after the # must be in the range 0 -
0x10ffff, so there can be up to 7 decimal digits or 6 hex digits.

What about &#x...; escapes?

What about /&#(?:\\d{1,7}|[xX][0-9a-fA-F]{1,6})(?![;0-9a-fA-F])/ as the pattern.

[gagan.goku, 2010-08-10 09:11:43]

http://codereview.appspot.com/1866052/diff/9001/10002
File src/com/google/caja/parser/html/Html5ElementStack.java (right):

http://codereview.appspot.com/1866052/diff/9001/10002#newcode523
src/com/google/caja/parser/html/Html5ElementStack.java:523: return
text.replaceAll("(\\b|\\B)(&#\\d{2,4});?", "$2;");
On 2010/08/09 19:12:38, Kuntal Loya wrote:
> Is \b|\B required?
> 
> Btw, there can be any number of digits (dont know the upper limit) after &#,
> which if have a ; missing at the end, would still be a valid escape character.

Now using the regex suggested by Mike.

http://codereview.appspot.com/1866052/diff/9001/10002#newcode523
src/com/google/caja/parser/html/Html5ElementStack.java:523: return
text.replaceAll("(\\b|\\B)(&#\\d{2,4});?", "$2;");
On 2010/08/09 19:34:49, MikeSamuel wrote:
> On 2010/08/09 19:12:38, Kuntal Loya wrote:
> > Is \b|\B required?
> > 
> > Btw, there can be any number of digits (dont know the upper limit) after &#,
> > which if have a ; missing at the end, would still be a valid escape
character.
> 
> To answer Kuntal's question, the value after the # must be in the range 0 -
> 0x10ffff, so there can be up to 7 decimal digits or 6 hex digits.
> 
> What about &#x...; escapes?
> 
> What about /&#(?:\\d{1,7}|[xX][0-9a-fA-F]{1,6})(?![;0-9a-fA-F])/ as the
pattern.

Done.
Changed it a little bit to save the matched part including the "&#" part

[anupama.dutta, 2010-08-10 09:21:58]

Good tests :)
LGTM.

[Kuntal Loya, 2010-08-10 09:58:50]

http://codereview.appspot.com/1866052/diff/26001/27001
File tests/com/google/caja/parser/html/Html5ElementStackTest.java (right):

http://codereview.appspot.com/1866052/diff/26001/27001#newcode91
tests/com/google/caja/parser/html/Html5ElementStackTest.java:91: + "&#xaaaaaaa
&#123a &#123";
expected for &#123a should be &#123a;

[Kuntal Loya, 2010-08-10 10:01:21]

LGTM

http://codereview.appspot.com/1866052/diff/26001/27001
File tests/com/google/caja/parser/html/Html5ElementStackTest.java (right):

http://codereview.appspot.com/1866052/diff/26001/27001#newcode91
tests/com/google/caja/parser/html/Html5ElementStackTest.java:91: + "&#xaaaaaaa
&#123a &#123";
On 2010/08/10 09:58:50, Kuntal Loya wrote:
> expected for &#123a should be &#123a;

Sorry, thought it to be &#x123a

[gagan.goku, 2010-08-10 10:02:44]

Thanks Anupama, Kuntal for the review.

http://codereview.appspot.com/1866052/diff/26001/27001
File tests/com/google/caja/parser/html/Html5ElementStackTest.java (right):

http://codereview.appspot.com/1866052/diff/26001/27001#newcode91
tests/com/google/caja/parser/html/Html5ElementStackTest.java:91: + "&#xaaaaaaa
&#123a &#123";
On 2010/08/10 09:58:50, Kuntal Loya wrote:
> expected for &#123a should be &#123a;

I think you meant "&#x123a" (which should be rewritten as "ሺ")
&#123a should be rewritten as "{a"

and both of these are consistent with what webkit does.

[gagan.goku, 2010-08-11 18:37:53]

On 2010/08/10 10:02:44, gagan.goku wrote:
> Thanks Anupama, Kuntal for the review.
> 
> http://codereview.appspot.com/1866052/diff/26001/27001
> File tests/com/google/caja/parser/html/Html5ElementStackTest.java (right):
> 
> http://codereview.appspot.com/1866052/diff/26001/27001#newcode91
> tests/com/google/caja/parser/html/Html5ElementStackTest.java:91: + "&#xaaaaaaa
> &#123a &#123";
> On 2010/08/10 09:58:50, Kuntal Loya wrote:
> > expected for &#123a should be &#123a;
> 
> I think you meant "&#x123a" (which should be rewritten as "ሺ")
> &#123a should be rewritten as "{a"
> 
> and both of these are consistent with what webkit does.

Hi Mike

This cl is ready for you to take another look.

Thanks
Gagan

[MikeSamuel, 2010-08-12 03:37:09]

http://codereview.appspot.com/1978041/show is a caja patch that does
do entity fixup for numeric entities and known named entities like
&amp; and &LT; but not for non-entities like "bar" in <a
href="?foo=x&bar=y">.

It's also careful not to try to fix entities in CDATA like
<script>if (lt&gt) { ... }</script>

2010/8/11  <gagan.goku@gmail.com>:
> On 2010/08/10 10:02:44, gagan.goku wrote:
>>
>> Thanks Anupama, Kuntal for the review.
>
>> http://codereview.appspot.com/1866052/diff/26001/27001
>> File tests/com/google/caja/parser/html/Html5ElementStackTest.java
>
> (right):
>
>> http://codereview.appspot.com/1866052/diff/26001/27001#newcode91
>> tests/com/google/caja/parser/html/Html5ElementStackTest.java:91: +
>
> "&#xaaaaaaa
>>
>> &#123a &#123";
>> On 2010/08/10 09:58:50, Kuntal Loya wrote:
>> > expected for &#123a should be &#123a;
>
>> I think you meant "&#x123a" (which should be rewritten as "&#x123a;")
>> &#123a should be rewritten as "&#123;a"
>
>> and both of these are consistent with what webkit does.
>
> Hi Mike
>
> This cl is ready for you to take another look.
>
> Thanks
> Gagan
>
> http://codereview.appspot.com/1866052/show
>

[gagan.goku, 2010-08-12 09:03:57]

Took a look at your change, it looks nice :) I just have small comments for
you.


On Thu, Aug 12, 2010 at 9:06 AM, Mike Samuel <mikesamuel@gmail.com> wrote:

> http://codereview.appspot.com/1978041/show is a caja patch that does
> do entity fixup for numeric entities and known named entities like
> &amp; and &LT; but not for non-entities like "bar" in <a
> href="?foo=x&bar=y">.
>
> It's also careful not to try to fix entities in CDATA like
> <script>if (lt&gt) { ... }</script>
>
> 2010/8/11  <gagan.goku@gmail.com>:
> > On 2010/08/10 10:02:44, gagan.goku wrote:
> >>
> >> Thanks Anupama, Kuntal for the review.
> >
> >> http://codereview.appspot.com/1866052/diff/26001/27001
> >> File tests/com/google/caja/parser/html/Html5ElementStackTest.java
> >
> > (right):
> >
> >> http://codereview.appspot.com/1866052/diff/26001/27001#newcode91
> >> tests/com/google/caja/parser/html/Html5ElementStackTest.java:91: +
> >
> > "&#xaaaaaaa
> >>
> >> &#123a &#123";
> >> On 2010/08/10 09:58:50, Kuntal Loya wrote:
> >> > expected for &#123a should be &#123a;
> >
> >> I think you meant "&#x123a" (which should be rewritten as "&#x123a;")
> >> &#123a should be rewritten as "&#123;a"
> >
> >> and both of these are consistent with what webkit does.
> >
> > Hi Mike
> >
> > This cl is ready for you to take another look.
> >
> > Thanks
> > Gagan
> >
> > http://codereview.appspot.com/1866052/show
> >
>

[gagan.goku, 2010-08-28 03:27:47]

Closing this patch for now as http://codereview.appspot.com/1978041/show  is
supposed to take care of this issue.