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

RESOLVED FIXED in Firefox 65

Status

()

defect
P5
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: pmenzel+bugzilla.mozilla.org, Assigned: jorgk, Mentored)

Tracking

52 Branch
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(6 attachments, 3 obsolete attachments)

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.
The conversion from plain text to HTML for display happens in a Mozilla core library.
Component: Untriaged → Networking
Product: Thunderbird → Core
If you provide a test case (preferably as a unit test in netwerk/test/unit/test_mozTXTToHTMLConv.js) we can try to fix it.
Flags: needinfo?(jorgk)
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.
Flags: needinfo?(jorgk)
Priority: -- → P5
Whiteboard: [necko-triaged]
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.
Mentor: valentin.gosu
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.
Assignee: nobody → valentin.gosu
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee: valentin.gosu → nobody
Status: ASSIGNED → NEW
See also Bug 106028
Wow, with a patch and 71 comments :-(
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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
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.
Attachment #9028731 - Flags: review?(valentin.gosu)
Attachment #9028731 - Flags: review?(valentin.gosu) → review+
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 ;-)
Attachment #9028739 - Flags: review?(valentin.gosu)
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.
Attachment #9028739 - Flags: review?(valentin.gosu) → review+
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.
Attachment #9026713 - Attachment is obsolete: true
Attachment #9028748 - Flags: review?(valentin.gosu)
Sorry, fixed commit message.
Attachment #9028748 - Attachment is obsolete: true
Attachment #9028748 - Flags: review?(valentin.gosu)
Attachment #9028749 - Flags: review?(valentin.gosu)
Attachment #9028749 - Flags: review?(valentin.gosu) → review+
These are Simon's tests from bug 106028.
Attachment #9028752 - Flags: review?(valentin.gosu)
Comment on attachment 9028752 [details] [diff] [review]
1505911-struct-testing.patch

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

The more tests the better. Thanks!
Attachment #9028752 - Flags: review?(valentin.gosu) → review+
I fixed the nits. Carrying forward Valentin's r+.
Attachment #9028739 - Attachment is obsolete: true
Attachment #9028755 - Flags: review+
OK, done here. Can you get your patch in attachment 9028677 [details] [diff] [review] reviewed please so we can land the lot.
Attachment #9028677 - Flags: review?(juhsu)
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?
Attachment #9028677 - Flags: review?(juhsu) → review+
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.
(In reply to Jorg K (GMT+1) from comment #19)
> You mean: nsDependentSubstring?
> 

Right, Thanks.
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
(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 :)
Very nice, big thanks!!
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e4612d760fef
Adapt to mozITXTToHTMLConv API changes (AString instead of wstring) in C-C. r=valentin
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!
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.
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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #9029160 - Flags: review?(valentin.gosu)
Attachment #9029160 - Flags: review?(valentin.gosu) → review+
https://hg.mozilla.org/mozilla-central/rev/0ad042243cf0
Status: REOPENED → RESOLVED
Closed: 8 months ago8 months ago
Resolution: --- → FIXED
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
Blocks: 1512647
You need to log in before you can comment on or make changes to this bug.