[Messages][RTL] thread information in thread list view is not showing the ellipsis in the correct location for LTR names/message

VERIFIED FIXED in Firefox OS v2.2

Status

P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

unspecified
2.2 S8 (20mar)
ARM
Gonk (Firefox OS)
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 8571257 [details]
Screen Shot 2015-03-02 at 17.49.11.png

Please see the screenshot, the ellipsis for LTR contact/message is displayed at worng side when system lang is RTL.

We proposed a hack in bug 1130305, but we might face more complicated situation like multiple contacts with LTR/RTL contacts mixed up. Hi Ahmed, do you have any suggestion about the LTR/RTL contacts mixed up case? For example, we have a multiple contacts thread(Julien, Ahmed, عمر الخيام) need to display is RTL. Could we display Julien, Ahmed, الخيام... when the space is limited, or it's inappropriate because it's wrong ellipsis direction for RTL contact?
(Assignee)

Updated

4 years ago
Flags: needinfo?(nefzaoui.ahmed)
(Assignee)

Comment 1

4 years ago
Remove the ni? since Ahmed already left some comments in bug 1094131. Althought we might have no better solution for group contact, fixing single contact and message body is relatively easy and less side effect. I will create a patch for this later.
Flags: needinfo?(nefzaoui.ahmed)
Created attachment 8575755 [details] [review]
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master
(Assignee)

Comment 3

4 years ago
Comment on attachment 8575755 [details] [review]
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master

Hi Oleg, this patch is for fixing the single contact and body ellipsis issue. For the text body part I have to change the container's display to flex and some list layout overrride because of display: block changes for text part. You can raise your concern if you think overriding BB is inappropriate, thanks.
Attachment #8575755 - Flags: review?(azasypkin)
(In reply to Steve Chung [:steveck] from comment #3)
> Comment on attachment 8575755 [details] [review]
> [gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master
> 
> Hi Oleg, this patch is for fixing the single contact and body ellipsis
> issue. For the text body part I have to change the container's display to
> flex and some list layout overrride because of display: block changes for
> text part. You can raise your concern if you think overriding BB is
> inappropriate, thanks.

Looks good, left one question and one cosmetic suggestion at GitHub. Keeping r? for now.
Comment on attachment 8575755 [details] [review]
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master

Hey Steve, since I'll be on PTO the following two weeks I'm forwarding review to Julien.
Attachment #8575755 - Flags: review?(azasypkin) → review?(felash)
(Assignee)

Comment 6

4 years ago
Hi Julien, I updated the patch based on Oleg's suggestion: https://github.com/mozilla-b2g/gaia/pull/28782#discussion_r26350131. Actually both margin-start(in list.css) and margin-end(in message) were set but margin-start does not working. Seems the margin: 0px will clear the margin-start in list. The following commit is simply some adjustment for margin only.
Comment on attachment 8575755 [details] [review]
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master

r=me

I tried various cases but couldn't make it fail.

Can you please file a separate bug to remove [data-type=list] selectors ?
Attachment #8575755 - Flags: review?(felash) → review+
blocking-b2g: --- → 2.2?
Blocks: 1064569
Priority: -- → P2
(Assignee)

Updated

4 years ago
Assignee: nobody → schung
Triage: Ellipsis inconsistency. Agreed on blocking 2.2 on it.
blocking-b2g: 2.2? → 2.2+
(Assignee)

Comment 9

4 years ago
Thanks for the review! I agree these css rules looks not so pleasant. I filed bug 1144612 for refactor this issue.

In master: https://github.com/mozilla-b2g/gaia/commit/a938f3abd40b0367b3f91ca8777a751b52b36d8a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 10

4 years ago
Comment on attachment 8575755 [details] [review]
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): RTL feature request
[User impact] if declined: Wrong contact/message text ellipsis direction in thread list view 
[Testing completed]: N/A
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A
Attachment #8575755 - Flags: approval-gaia-v2.2?(bbajaj)
Comment on attachment 8575755 [details] [review]
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master

Clearing approval request until this sticks.
Attachment #8575755 - Flags: approval-gaia-v2.2?(bbajaj)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Should be easy to fix, sorry for this :/
Created attachment 8579787 [details] [review]
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master
(Assignee)

Comment 19

4 years ago
Comment on attachment 8579787 [details] [review]
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master

I deeply apologise for this negligence... I made a little adjustment to the unit test to make the test less fragile about the dom structure changes, since we might remove this workaround some day.
Flags: needinfo?(schung)
Attachment #8579787 - Flags: review?(felash)
Comment on attachment 8579787 [details] [review]
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master

lgtm thanks !
r+
Attachment #8579787 - Flags: review?(felash) → review+
(Assignee)

Comment 21

4 years ago
In master: https://github.com/mozilla-b2g/gaia/commit/7e3e0d51985a42c69eeede41397e4c0c8b175475
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Attachment #8575755 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Comment on attachment 8579787 [details] [review]
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): RTL feature request
[User impact] if declined: Wrong contact/message text ellipsis direction in thread list view 
[Testing completed]: N/A
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A
Attachment #8579787 - Flags: approval-gaia-v2.2?(bbajaj)

Updated

4 years ago
Attachment #8579787 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/d942a8b0658a77ed6efe3e2184bdd2eff4410e97
status-b2g-v2.2: affected → fixed
Target Milestone: --- → 2.2 S8 (20mar)
This issue is verified fixed on the latest Nightly Flame 3.0 and 2.2 builds.

Actual Results: The ellipsis is on the correct side when viewing an LTR name/message while the phone is set to an RTL langauge.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150323010204
Gaia: 9b6f3024e4d0e62dd057231f4b14abe1782932ab
Gecko: e730012260a4
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150323002504
Gaia: 7f367fc98ffdd183f21d2cdfe20556ab877ece34
Gecko: 3ea0eaeda353
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)

Comment 25

4 years ago
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15930/
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.