1505911 - UTF-8 string not displayed as bold when marked up with asterisks

Status: RESOLVED FIXED

(Core :: Networking, defect, P5)

Description

[Paul Menzel]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0

Steps to reproduce:

Use 52.9.1 and mark up the string *Test test Ⅱ* with asterisks.


Actual results:

Sending a plain text message with the string *Test test Ⅱ* in it, Mozilla Thunderbird does not display the enclosed string in bold font. *Test test* works.


Expected results:

Test test Ⅱ should be displayed with bold font.

Comment 1

[Jorg K (CEST = GMT+2)]

The conversion from plain text to HTML for display happens in a Mozilla core library.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

If you provide a test case (preferably as a unit test in netwerk/test/unit/test_mozTXTToHTMLConv.js) we can try to fix it.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1505911-more-tests-mozTXTToHTMLConv.patch (v1)

Here you go. There was zero testing for any of this, so I've added some. Not exhaustive. You can see which test should give a better result.

Briefly looking at
https://searchfox.org/mozilla-central/rev/8f89901f2d69d9783f946a7458a6d7ee70635a94/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp#1102
I can't really see what's going wrong. It's all done with (wide) nsString or char16_t, so why does a non-ASCII character like ß (German sz) make a difference? Hmm.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

From what I can tell the bug seems to be in mozTXTToHTMLConv::ItMatchesDelimited()
There are a bunch of calls to IsAsciiAlpha() - it tries to make sure that the characters * these characters * between `*` tokens are not delimiters themselves. But isAsciiAlpha || isAsciiDigit isn't exactly right, as it doesn't match things like Ⅱ or ß.

I don't have time to fix it right now, but I'll gladly review a patch and mentor someone through it.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: Change interfaces to use AString instead of wstring

It would be great if the fix for this bug would also incorporate this patch. Using wchar here seems like a huge pain, and we should move to using AString where possible.

Comment 6

[Alfred Peters [:infofrommozilla]]

See also Bug 106028

Comment 7

[Jorg K (CEST = GMT+2)]

Wow, with a patch and 71 comments :-(

Comment 8

[Jorg K (CEST = GMT+2)]

OK, I'll take the bug. Let's use a large chunk of the patch in bug 106028 with added tests. I'll wait until bug 1509493 gets merged to avoid merge conflicts.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: C-C: 1505911-string-API-changes.patch

OK, I assume you'll get your patch reviewed by a Netwerk peer. Here's the C-C part. The fix is going to come in a minute.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Simon's patches from bug 106028 and bug 415209 combined

OK, this combines Simon's patches from bug 106028 and bug 415209. Of course I left him as the original author. The tests in the next patch will show whether it works ;-)

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9028739 [details] [diff] [review]
Simon's patches from bug 106028 and bug 415209 combined

Review of attachment 9028739 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
@@ +566,5 @@
>  }
>  
> +static inline bool IsAlpha(const uint32_t aChar)
> +{
> +  return (mozilla::unicode::GetGenCategory(aChar) == nsUGenCategory::kLetter);

nit: no parentheses required around the expression.

@@ +568,5 @@
> +static inline bool IsAlpha(const uint32_t aChar)
> +{
> +  return (mozilla::unicode::GetGenCategory(aChar) == nsUGenCategory::kLetter);
> +}
> +	

trailing whitespace

@@ +571,5 @@
> +}
> +	
> +static inline bool IsDigit(const uint32_t aChar)
> +{
> +  return (mozilla::unicode::GetGenCategory(aChar) == nsUGenCategory::kNumber);

nit: no parentheses

@@ +603,5 @@
>  
> +  if ((before == LT_ALPHA && !IsAlpha(text0)) ||
> +      (before == LT_DIGIT && !IsDigit(text0)) ||
> +      (before == LT_DELIMITER &&
> +       (IsAlpha(text0) || IsDigit(text0) || text0 == *rep)) ||

The conditions for before and after are the same. It seems to be that we could use a helper function.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1505911-more-tests-mozTXTToHTMLConv.patch

Here are some glyph tests. Simon's struct tests are more exhaustive. I'll attach them in a separate patch to maintain him as the original author.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1505911-more-tests-mozTXTToHTMLConv.patch (v2b)

Sorry, fixed commit message.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1505911-struct-testing.patch

These are Simon's tests from bug 106028.

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9028752 [details] [diff] [review]
1505911-struct-testing.patch

Review of attachment 9028752 [details] [diff] [review]:
-----------------------------------------------------------------

The more tests the better. Thanks!

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: Simon's patches from bug 106028 and bug 415209 combined

I fixed the nits. Carrying forward Valentin's r+.

Comment 17

[Jorg K (CEST = GMT+2)]

OK, done here. Can you get your patch in attachment 9028677 [details] [diff] [review] reviewed please so we can land the lot.

Comment 18

[Junior [inactive]]

Comment on attachment 9028677 [details] [diff] [review]
Change interfaces to use AString instead of wstring

Review of attachment 9028677 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
@@ +382,5 @@
>    txtURL.StripWhitespace();
>  
>    // FIX ME
>    nsAutoString temp2;
> +  ScanTXT(Substring(aInString, descstart), ~kURLs /*prevents loop*/ & whathasbeendone, temp2);

Can we use nsDependentString since ScanTXT uses const nsAString?

Comment 19

[Jorg K (CEST = GMT+2)]

You mean: nsDependentSubstring?

Valentin, could you push the four M-C patches together. I don't have inbound locally and I'd like to avoid writing instructions for the sheriffs.

Comment 20

[Junior [inactive]]

(In reply to Jorg K (GMT+1) from comment #19)
> You mean: nsDependentSubstring?
> 

Right, Thanks.

Comment 21

[Jorg K (CEST = GMT+2)]

Looks like bug 1511181 shredded right through the C++ changes here:

applying base-4f5626442be0
patching file netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
Hunk #1 FAILED at 30
Hunk #2 FAILED at 377
Hunk #3 FAILED at 643
Hunk #4 FAILED at 757
Hunk #5 FAILED at 1067
Hunk #6 FAILED at 1156
Hunk #7 FAILED at 1192
Hunk #8 FAILED at 1291
Hunk #9 FAILED at 1351
9 out of 9 hunks FAILED -- saving rejects to file netwerk/streamconv/converters/mozTXTToHTMLConv.cpp.rej
patching file netwerk/streamconv/converters/mozTXTToHTMLConv.h
Hunk #1 FAILED at 28
Hunk #2 FAILED at 106
Hunk #3 FAILED at 241
Hunk #4 FAILED at 271
4 out of 4 hunks FAILED -- saving rejects to file netwerk/streamconv/converters/mozTXTToHTMLConv.h.rej

Comment 22

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jorg K (GMT+1) from comment #21)
> Looks like bug 1511181 shredded right through the C++ changes here:

I'll try to land them when the trees open.
I think my patch was the biggest problem when rebasing, but I'll make it work :)

Comment 23

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1805a92cf563c0352f16e369d8fea2cfdda32d5

Comment 24

[Jorg K (CEST = GMT+2)]

Very nice, big thanks!!

Comment 25

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc73687c1ef6d72850b1e8241104708fd1bca35
Bug 1505911 - Change interfaces to use AString instead of wstring r=juhsu

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c40ca38958fc08fb4289dd1eb45b7fd7258ff99
Bug 1505911 - Use IsAlpha/IsDigit instead of IsAsciiAlpha/IsAsciiDigit for full Unicode support. r=valentin

https://hg.mozilla.org/integration/mozilla-inbound/rev/f64d8eeab965ad962b09952d5e3978db0d4b8199
Bug 1505911 - Add glyph testing to test_mozTXTToHTMLConv.js. r=valentin

https://hg.mozilla.org/integration/mozilla-inbound/rev/dec172d2c7fd901849b9496601f73483b31f12b0
Bug 1505911 - Add struct testing to test_mozTXTToHTMLConv.js. r=valentin

Comment 26

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7fc73687c1ef
https://hg.mozilla.org/mozilla-central/rev/0c40ca38958f
https://hg.mozilla.org/mozilla-central/rev/f64d8eeab965
https://hg.mozilla.org/mozilla-central/rev/dec172d2c7fd

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e4612d760fef
Adapt to mozITXTToHTMLConv API changes (AString instead of wstring) in C-C. r=valentin

Comment 28

[Ben Bucksch (:BenB)]

Hey Jörg, thanks for these changes! I like that you changed this to nsAString. This class had originally been written only with string classes, and then somebody else went in and converted it all to wide char*, much to my dismay and opposition. I am glad this finally got converted back to string classes. I'm also happy that AString now is API-rich enough to do all this. 15 years ago, the abstract interface had only few function. I'm glad this works out so well now.

I looked at the changes only superficially, but what I saw all looked good. Great improvement, thanks!

Just a little request to everybody involved: If there's a patch for mozTXTToHTMLConv, could you please needinfo me, so that I can take a look at it? That would be appreciated. Thank you!

Comment 29

[Jorg K (CEST = GMT+2)]

You were looking at it since 2013 over in bug 106028 despite 17(!!) duplicates and 19 votes :-( - Not the best service to our international users.

Comment 30

[Jorg K (CEST = GMT+2)]

Looks like we caused some test failures here:

TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:comm/calendar/test/unit/test_ltninvitationutils.js | createInvitationOverlay_test - [createInvitationOverlay_test : 385] (test #1) -
"Go to<a xmlns=\\"http://www.w3.org/1999/xhtml\\" class=\\"moz-txt-link-freetext\\" href=\\"https://www.example.net\\">https://www.example.net</a> if you can." ==
"Go to <a xmlns=\\"http://www.w3.org/1999/xhtml\\" class=\\"moz-txt-link-freetext\\" href=\\"https://www.exa

Somehow the result is truncated, or there is an extra space now? I'll take a look.

Comment 31

[Jorg K (CEST = GMT+2)]

There's nothing truncated, but there is a space missing:
Input: 
Go to https://www.example.net if you can.

Output:
Go to<a xmlns="http://www.w3.org/1999/xhtml" class="moz-txt-link-freetext" href="https://www.example.net">https://www.example.net</a> if you can.

Correct would be:
Go to <a xmlns="http://www.w3.org/1999/xhtml" class="moz-txt-link-freetext" href="https://www.example.net">https://www.example.net</a> if you can.

I think the error is here:
https://hg.mozilla.org/mozilla-central/rev/7fc73687c1ef#l1.31

Comment 32

[Jorg K (CEST = GMT+2)]

Attachment: 1505911-follow-up.patch

Looks like an error crept in which the M-C tests didn't pick up as they only check the produced URL but not the overall output.

Valentin, this makes our tree red, so can you please review and push this quickly.

Comment 33

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad042243cf0ea701257465d72e780d4dbfc0225
Bug 1505911 - Follow-up: fix error in part 1. r=valentin

Comment 34

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/0ad042243cf0

Comment 35

[Jorg K (CEST = GMT+2)]

I've taken the relevant part to TB 60 ESR for TB 60.4 ESR:
https://hg.mozilla.org/releases/mozilla-esr60/rev/42791ed044618b41dac82fe85d0c10655511e2ee on THUNDERBIRD_60_VERBRANCH

Comment 36

[Intermittent Failures Robot]

4 failures in 2808 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:
* comm-central: 4

Platform breakdown:
* windows10-64: 2
* windows7-32: 2

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1505911&startday=2018-11-26&endday=2018-12-02&tree=all