Closed Bug 1138350 Opened 9 years ago Closed 9 years ago

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

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

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

Status:
VERIFIED FIXED
Milestone:
2.2 S8 (20mar)
Project Flags:
blocking-b2g 2.2+
Tracking Flags:
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: steveck, Assigned: steveck)

References

Details

Attachments

(2 files, 1 obsolete file)

Screen Shot 2015-03-02 at 17.49.11.png
54.65 KB, image/png
Details
[gaia] steveck-chung:message-thread-list-RTL > mozilla-b2g:master
46 bytes, text/x-github-pull-request
julienw
: review+
bajaj
: approval-gaia-v2.2+
Details | Review 
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?
Flags: needinfo?(nefzaoui.ahmed)
See Also: → 1094131, 883884
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)
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)
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?
Priority: -- → P2
Assignee: nobody → schung
Triage: Ellipsis inconsistency. Agreed on blocking 2.2 on it.
blocking-b2g: 2.2? → 2.2+
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
Closed: 9 years ago
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
Resolution: --- → FIXED
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)
Should be easy to fix, sorry for this :/
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+
In master: https://github.com/mozilla-b2g/gaia/commit/7e3e0d51985a42c69eeede41397e4c0c8b175475
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Attachment #8575755 - Attachment is obsolete: true
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)
Attachment #8579787 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
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)
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.

Attachment

General

Creator:
Created:
Updated:
Size: