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

RESOLVED FIXED in Firefox 65

Status

()

defect
P5
normal
RESOLVED FIXED
7 months ago
6 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)

Reporter

Description

7 months ago
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.
Assignee

Comment 1

7 months ago
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)
Assignee

Comment 3

6 months ago
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

Comment 6

6 months ago
See also Bug 106028
Assignee

Comment 7

6 months ago
Wow, with a patch and 71 comments :-(
Assignee

Comment 8

6 months ago
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
Assignee

Comment 9

6 months ago
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+
Assignee

Comment 10

6 months ago
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+
Assignee

Comment 12

6 months ago
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)
Assignee

Comment 13

6 months ago
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+
Assignee

Comment 14

6 months ago
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+
Assignee

Comment 16

6 months ago
I fixed the nits. Carrying forward Valentin's r+.
Attachment #9028739 - Attachment is obsolete: true
Attachment #9028755 - Flags: review+
Assignee

Comment 17

6 months ago
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+
Assignee

Comment 19

6 months ago
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.
Assignee

Comment 21

6 months ago
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 :)
Assignee

Comment 24

6 months ago
Very nice, big thanks!!

Comment 27

6 months ago
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!
Assignee

Comment 29

6 months ago
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.
Assignee

Comment 30

6 months ago
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.
Assignee

Comment 31

6 months ago
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
Assignee

Updated

6 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 32

6 months ago
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+

Comment 34

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0ad042243cf0
Status: REOPENED → RESOLVED
Last Resolved: 6 months ago6 months ago
Resolution: --- → FIXED
Assignee

Comment 35

6 months ago
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 hidden (Intermittent Failures Robot)
Blocks: 1512647
You need to log in before you can comment on or make changes to this bug.