1145546 - [RTL] input fields don't autoscroll to keep the caret in view with unicode-bidi: [-moz-]plaintext

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Sue]

Attachment: video.MP4

[1.Description]:
[RTL][v3.0][Settings] When we log in a Firefox account with long email address, the first character will not be indented when we input the long email address.
See attachment:video.MP4

[2.Testing Steps]: 
1. Set system language as Arabic.
2. Launch Settings -> Firefox Accounts -> Create Account or Sign In.
3. Input a long email address.

[3.Expected Result]: 
3. The first character should be indented when we input a long email address.

[4.Actual Result]: 
3. The first character will not be indented when we input a long email address.

[5.Reproduction build]: 
Flame 2.2 build: unaffected
Build ID               20150319002500
Gaia Revision          9043c11f699c15bb6072422d1dad6518d1b5ddda
Gaia Date              2015-03-19 01:40:44
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c0442d170bec
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150319.042028
Firmware Date          Thu Mar 19 04:20:38 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 build: affected
Build ID               20150319160212
Gaia Revision          c39e15f631de80c69467fda0d4ea0bcda9e194ca
Gaia Date              2015-03-18 19:30:04
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/cbd0efcd976c
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150319.193329
Firmware Date          Thu Mar 19 19:33:42 EDT 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

Comment 1

[Delphine Lebédel [:delphine - Use needinfo]]

Sue: I think you got the description of the bug wrong. Or maybe this is a language issue? "Indenting"
here doesn't make sense from the video I'm looking at. I will change the title of this bug to reflect the issue seen in the video. Please refile a bug about indenting if there is in fact a bug somewhere.

Triage P2 -- This denies information from users who have a very long email address, so nominating as well

Comment 2

[Sue]

(In reply to Delphine Lebédel [:delphine - use need info] from comment #1)
> Sue: I think you got the description of the bug wrong. Or maybe this is a
> language issue? "Indenting"
> here doesn't make sense from the video I'm looking at. I will change the
> title of this bug to reflect the issue seen in the video. Please refile a
> bug about indenting if there is in fact a bug somewhere.
> 
> Triage P2 -- This denies information from users who have a very long email
> address, so nominating as well

Hi Delphine,
Thanks. :)
This issue is talking about, when users typed a very long email address, the first input characters can't move to left, so users can't see what they are inputting.

Comment 3

[howie [:howie]]

Triage: not Gaia bug, more like layout or platform issue. Put to layout first, please help to change component if it's not correct, thanks.

Comment 4

[Simon Montagu :smontagu]

This was presumably exposed by bug 1137021, but it seems like a layout bug with autoscrolling of input fields. Normally the field scrolls the text while the user types, and keeps the point in the text where characters are being added in view, and this Just Works with all combinations of right-to-left and left-to-right text and field direction. However it stops working with unicode-bidi: -moz-plaintext where the direction of the text is the opposite of the direction of the field.

Comment 5

[Simon Montagu :smontagu]

Attachment: Test case with different combinations of directions

Comment 6

[Simon Montagu :smontagu]

Attachment: Patch

N.B.1: There is a very similar bug with input fields with vertical text, but that can wait for its own patch and not confuse the issue here.

N.B.2: I'm not sure whether it would be a good idea to change nsLayoutUtils::GetScrolledRect's |aDirection| to take NS_BIDILTR/NS_BIDIRTL instead of NS_STYLE_DIRECTION_LTR/NS_STYLE_DIRECTION_RTL. It would make this callsite significantly clearer, but the other callsite in GetScrollRectSizeForOverflowVisibleFrame in dom/base/Element.cpp a bit less so.

Or on second thoughts, I suppose the upcoming fix to the vertical text issues will require passing in a WritingMode anyway, so that will become moot.

Comment 7

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c309c399dfad

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c309c399dfad

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request b2g37 approval on this when you get a chance.

Comment 10

[Lancy(Leave from Mozilla)]

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

Comment 11

[Lancy(Leave from Mozilla)]

Attachment: Verify1_Flame3.0_Pass.mp4

According to the steps in Comment 0, this issue is verified pass on latest Flame 3.0 build.
See attachment: Verify1_Flame3.0_Pass.mp4
Reproducing Rate:0/10

Flame 3.0 build: 
Build ID               20150327160203
Gaia Revision          9cc496cecc37d7a29f9279827cdf6e4891211f67
Gaia Date              2015-03-27 13:55:18
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/44e454b5e93b
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150327.192632
Firmware Date          Fri Mar 27 19:26:42 EDT 2015
Bootloader             L1TC000118D0

Comment 12

[Simon Montagu :smontagu]

Comment on attachment 8583144 [details] [diff] [review]
Patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): The underlying layout bug has apparently existed ever since support for unicode-bidi: plaintext was introduced in bug 662288, but it has appeared in specific fields in Gaia UI since bug 1137021
User impact if declined: Users in RTL languages unable to see text while typing if it exceeds the width of the text field
Testing completed: Baked on trunk since 2015-03-26. Tested with input fields in all combinations of direction and rtl/ltr languages.
Risk to taking this patch (and alternatives if risky): The patch only affects fields with unicode-bidi: plaintext and should have no side-effects outside that.
String or UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/aa92e146b557

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Non-unified bustage follow-up:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abea26b5a89f
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8133b19e079d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/abea26b5a89f

Comment 16

[Lancy(Leave from Mozilla)]

Attachment: Verify2_Flame2.2_Pass.mp4

According to the steps in Comment 0, this issue is verified pass on latest Flame 2.2 build.
See attachment: Verify2_Flame2.2_Pass.mp4
Reproducing Rate:0/10

Flame 2.2 build (Unaffected):
Build ID               20150401002624
Gaia Revision          8b3086ad3963f1707e2bee9094baccafffe161c4
Gaia Date              2015-03-31 21:48:06
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/20b67213a047
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150401.042225
Firmware Date          Wed Apr  1 04:22:36 EDT 2015
Bootloader             L1TC000118D0