1116315 - [SMS][RTL] Subject Line has improper overlap when input subject string is small.

Status: RESOLVED DUPLICATE of bug 1023702

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Martin Shuman [:Marty]]

Attachment: Message_Subject_Arabic_Screenshot.png

Description:
When adding only a few characters to an MMS subject line, there will be improper overlap with the original 'Subject' label string.

Note: If the user inputs a long Subject string, this issue will resolve itself until the next time a Subject line is added to an SMS.

Repro Steps:
1) Update a Flame to 20141229010215
2) Set the device language to Arabic ('عربي')
3) Open the Messages app
4) Compose a new message
5) Select the ellipsis ('...') in the top corner and tap 'Add subject'
6) In the subject line of the MMS, enter a single character 'T'


Actual:
There is a small remnant of the 'Subject' label still visible just to the right of the 'T'


Expected:
No part of the original 'Subject' label is still visible.

Environmental Variables:
Device: Flame 2.2
Build ID: 20141229010215
Gaia: bdedbaf9f18a43c091ede770407d68d38582fe29
Gecko: 8850aa0f5332
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


Repro frequency: 5/5
See attached: screenshot

Comment 1

[Julien Wajsberg [:julienw]]

Hey Ahmed, you might want to track this.

Comment 2

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Julien Wajsberg [:julienw] (PTO 12/25 -> 12/29) from comment #1)
> Hey Ahmed, you might want to track this.

Hey Julien, sorry for the trouble of having to ping two addresses..
Actually I tried this many times now and I'm still not able to reproduce this bug, could you please let me know if it's occurring with you?
Thanks!

Comment 3

[Julien Wajsberg [:julienw]]

Adding NI, I'll check today.

Ahmed, if one address is more suitable, just add :nefzaoui to the "good" address and remove from the other :)

Comment 4

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Julien Wajsberg [:julienw] (PTO 12/25 -> 12/29) from comment #3)
> Adding NI, I'll check today.
Great, Thanks!

> Ahmed, if one address is more suitable, just add :nefzaoui to the "good"
> address and remove from the other :)
Done.

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: testcase-arabic.html

Comment 6

[Julien Wajsberg [:julienw]]

Yes I actually reproduce it using the letter "T". It looks like an invalidation issue (like, part of the written arabic text is outside of the invalidation rectangle).

I attached a testcase that reproduces in Nightly for me.
If I further reduce the testcase, I still see the invalidation issue, but it disappears after some time.

I also remember I saw this even with some italic latin latters (like "l").

David, Robert, I've been told you could be the right people to look at this?


On B2G (I think it's using a different font), we can also see that the initial text is a little cut on the right. On my Firefox it's fine. Likely a different issue though.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This sounds like overflow areas being wrong.  (Overflow areas are rectangles that represent the area covered by the frame and all its descendants; there's one for painting (the one we care about here) and one for scrolling.)

There seem to be two places it could go wrong:
 (1) the text code producing incorrect overflow areas for the text
 (2) other code propagating those overflow areas incorrectly up the tree

I think (1) is probably more likely.  needinfo? to Jonathan, since he may be aware of similar problems or have other ideas.


I can reproduce with attachment 8542555 [details] on Linux.  What platform are you on?  However, some of the text code in question is somewhat platform-specific, so that doesn't necessarily mean it's the same bug you're hitting on B2G.

Comment 8

[Julien Wajsberg [:julienw]]

I'm on Linux too.

Comment 9

[Jonathan Kew [:jfkthame]]

Part, at least, of the problem here -- perhaps the whole of it -- is that a synthetic italic effect is being applied to the Arabic text (because the Arabic font does not have a true italic face available), and the text code does not take this into account when computing the ink bounding box of the glyphs, which needs to be contributed to the overflow area for painting.

This is a Graphics:Text bug that we should fix, as it could lead to invalidation/painting glitches in a variety of situations; however, in general the SMS app (or any other apps, for that matter) should avoid using italic styling with Arabic script; this is not an appropriate stylistic/design choice. "Italics" are a Latin-script-centric concept, and most non-Latin writing systems do not have the same tradition. (See also bug 1023702.)

Avoiding the inappropriate use of italics with the Arabic font would avoid the drawing glitch here.

Comment 10

[Jonathan Kew [:jfkthame]]

Filed bug 1116480 on the synthetic-italics bounding box issue that underlies this.

Comment 11

[Julien Wajsberg [:julienw]]

Thanks a lot for the insight.

I'm not sure how we can handle this at the CSS level. I guess that for arabic and hebrew and other RTL scripts we could do this, overriding all "font-style: italic", but what about hindi or bengali scripts? I guess italic styling is also wrong for them?

Another possibility, maybe saner: register an "italic" font face for all non-latin scripts that would just be the same font. I'm wondering if this could be done at the system level.

Kevin, how do you think we should do this?


(In reply to Jonathan Kew (:jfkthame) from comment #9)
> Part, at least, of the problem here -- perhaps the whole of it -- is that a
> synthetic italic effect is being applied to the Arabic text (because the
> Arabic font does not have a true italic face available), and the text code
> does not take this into account when computing the ink bounding box of the
> glyphs, which needs to be contributed to the overflow area for painting.

I suspect this is the same root cause for the additional issue I outlined in comment 6.

Comment 12

[Kevin Grandon :kgrandon]

If fixing bug 1116480 fixes this, then that would be my preferred approach. If we need something faster, I like your suggestion of registering an italic font face to use here if possible. I don't think we're going to be able to do UX cycles and remove italic font usage across the OS in the 2.2 timeframe.

Also I remember Tim looking into something similar before, so looping him in as well to get his thoughts.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: gfxTextRun::Measure needs to account for fake-italic transform when returning the bounding box

I think we need to do something like this to adjust for the skewed glyphs.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for textRun bounding box with fake italics

Here's a reftest to go with it. Without the patch, we get clipped drawing at the direction-run boundary in the testcase.

Comment 15

[Jonathan Kew [:jfkthame]]

Oops! Having filed bug 1116480 for the core issue here, that's where I meant to attach this patch and reftest. Moving them...

I still think it would be preferable to avoid the "italic Arabic" here, but that's a wider issue of Gaia UX design for non-Latin locales.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Kevin Grandon :kgrandon from comment #12)
> If fixing bug 1116480 fixes this, then that would be my preferred approach.
> If we need something faster, I like your suggestion of registering an italic
> font face to use here if possible. I don't think we're going to be able to
> do UX cycles and remove italic font usage across the OS in the 2.2 timeframe.
> 
> Also I remember Tim looking into something similar before, so looping him in
> as well to get his thoughts.

Bug 1116480 is moving and I don't have cycle available to contribute on communication burden to UX, in bug 1023702.

If we could verify bug 1116480 is the sole fix needed for fixing this issue, should we just dup the bug there? I don't think there is a patch to apply here.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Julien Wajsberg [:julienw] (PTO 12/25 -> 12/29) from comment #5)
> Created attachment 8542555 [details]
> testcase-arabic.html

BTW, Safari has the same bug, and it's fixed in Chrome.

Comment 18

[Julien Wajsberg [:julienw]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #16)
> (In reply to Kevin Grandon :kgrandon from comment #12)
> > If fixing bug 1116480 fixes this, then that would be my preferred approach.
> > If we need something faster, I like your suggestion of registering an italic
> > font face to use here if possible. I don't think we're going to be able to
> > do UX cycles and remove italic font usage across the OS in the 2.2 timeframe.
> > 
> > Also I remember Tim looking into something similar before, so looping him in
> > as well to get his thoughts.
> 
> Bug 1116480 is moving and I don't have cycle available to contribute on
> communication burden to UX, in bug 1023702.
> 
> If we could verify bug 1116480 is the sole fix needed for fixing this issue,
> should we just dup the bug there? I don't think there is a patch to apply
> here.

Yes, agreed, because bug 1023702 already exists, dupe to either bug 1116480 or bug 1023702.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This bug should be dup to bug 1116480 instead, but let's not spam people with bugmail.

Comment 21

[Alissa（leave from Mozilla）]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15932/