1269186 - [RTL] Search field in New Tab and about:home is not RTL'd

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Itiel]

User Agent: Mozilla/5.0 (Windows NT 10.0; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160421124000

Steps to reproduce:

1. Install RTL version of Firefox (e.g. Hebrew/Arabic)
2. Open a new tab


Actual results:

The search field is not suited for RTL:
1. The "Search" word is LTR'd
2. The search button (blue arrow) is pointing right
3. Typing RTL words in the search field will result in the first ~2 letters being partially hidden by the magnifying glass



Expected results:

1. The "Search" word is should be RTL'd
2. The search button (blue arrow) should point to the left
3. Typing RTL words in the search field should start AFTER the magnifying glass, not in it

Same issues occurs also in Firefox Developer Edition 48.0a2.

Screenshots attached.

Comment 1

[Itiel]

Attachment: Search button should point left.png

Comment 2

[Itiel]

Attachment: Letters hidden by the magnifying glass.png

Comment 3

[Itiel]

Attachment: Search word should be RTL'd.png

Comment 4

[Itiel]

Forgot to mention, I'm using Windows 10. Though I don't think this changes across different platforms.

Comment 5

[Itiel]

Needless to say, this also applies to about:home page.

Comment 6

[Itiel]

Same issue in the latest build of Develpoer Edition

Comment 7

[Loic]

Is it the same case with old versions of Firefox like 45 or 40?

Comment 8

[Itiel]

(In reply to Loic from comment #7)
> Is it the same case with old versions of Firefox like 45 or 40?

As far as I can remember, yes.
It was always like that if I'm not mistaken.

Comment 9

[Loic]

Ok, so it doesn't sound like a recent regression.

Comment 10

[Florian Quèze [:florian]]

(In reply to ItielMaN from comment #0)

> 3. Typing RTL words in the search field will result in the first ~2 letters
> being partially hidden by the magnifying glass

My patch attached to bug 1206709 fixes this third point.

Comment 11

[Itiel]

(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> (In reply to ItielMaN from comment #0)
> 
> > 3. Typing RTL words in the search field will result in the first ~2 letters
> > being partially hidden by the magnifying glass
> 
> My patch attached to bug 1206709 fixes this third point.

Great, thanks.
Hopefully with more patches to come :)

Comment 12

[Florian Quèze [:florian]]

(In reply to ItielMaN from comment #0)

> The search field is not suited for RTL:
> 1. The "Search" word is LTR'd

I'm not exactly sure of what's going on there. I see the issue on about:newtab but not on about:home. It's possible that using the ForceRTL add-on to test this isn't fully reliable. When it happens, pressing Ctrl+shift+X (or Command+shift+X on Mac) is enough to put the search field of about:newtab in the correct RTL direction.

> 2. The search button (blue arrow) is pointing right

Taking care of this second point in bug 1278546.

Bug 1206709 should also improve the search panels for RTL.

Comment 13

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)

> Bug 1206709 should also improve the search panels for RTL.

Err, I had already mentioned bug 1206709 in comment 10; it's bug 1204366 that I meant to mention in comment 12.

Comment 14

[Itiel]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> I'm not exactly sure of what's going on there. I see the issue on
> about:newtab but not on about:home. It's possible that using the ForceRTL
> add-on to test this isn't fully reliable.

That's why I prefer using the original RTL builds for tracking RTL issues..

> > 2. The search button (blue arrow) is pointing right
> 
> Taking care of this second point in bug 1278546.
> Bug 1206709 should also improve the search panels for RTL.

Thanks!

In what final Firefox version do you think these issues be will landed? (except bug 1204366, I saw it's scheduled for Firefox 50)

Comment 15

[Florian Quèze [:florian]]

(In reply to ItielMaN from comment #14)

> In what final Firefox version do you think these issues be will landed?
> (except bug 1204366, I saw it's scheduled for Firefox 50)

Most likely Firefox 50.

Comment 16

[Itiel]

#2 and #3, are confirmed to be fixed in Firefox 50.0a2, issue #1 remains.
Just to clarify, the "Search" word should be aligned to the right in RTL builds.

Comment 17

[Tomer Cohen :tomer]

(In reply to ItielMaN from comment #0)
> 1. The "Search" word is LTR'd
chrome://browser/content/newtab/newTab.xhtml

The placeholder text can't be easily controlled because the containing element has dir=auto and as far as I know the placeholder direction is undefined for this state. Unless we remove the dir=auto attribute, the best we can do is to re-style the placeholder, using something like #newtab-search-text::-moz-placeholder{direction: rtl;}, but it appears that dir=auto has higher priority over the placeholder direction (although I can control the font-size, color etc., it seems that it has no affect on direction).

I can see few ways to fix or workaround this issue:
a. Remove the placeholder text from affected locales (replace with an empty string).
b. Use text-align instead of direction in ::-moz-placeholder, and hope that no one will notice the different (as the string doesn't end with full stop it won't be noticeable).
c. Fix the W3C documents to define the behavior of the placeholder text more clearly.

Comment 18

[Khaled Hosny]

I looked into this issue and I think the problems comes from the use of dir="auto":
- It seems to not take the placeholder text into account,
- Also depending on the text the resolved direction for the input field can be LTR and in this case the margin padding will be swapped causing the text to overlap the icon.

I’m not sure if these are bugs in dir="auto" auto or its expected behavior, but dropping it and using unicode-bidi: plaintext; instead fixing the two issues I see.

Comment 19

[Khaled Hosny]

Attachment: Proposed patch

Comment 20

[Tomer Cohen :tomer]

Nice idea. It is indeed fix this issue (reproduced with DevTools).

Comment 21

[Khaled Hosny]

Attachment: Proposed patch

Home page needs the same treatment.

Comment 22

[Khaled Hosny]

(In reply to Tomer Cohen :tomer from comment #20)
> Nice idea. It is indeed fix this issue (reproduced with DevTools).

This is how I experimented with this, can’t wait until all Firefox UI is HTML and hackable with DevTools :)

Comment 23

[Tomer Cohen :tomer]

(In reply to Khaled Hosny from comment #22)
> > Nice idea. It is indeed fix this issue (reproduced with DevTools).
> This is how I experimented with this, can’t wait until all Firefox UI is
> HTML and hackable with DevTools :)
Actually DevTools can be used to hack XUL pages as well.

Comment 24

[Florian Quèze [:florian]]

(In reply to Khaled Hosny from comment #21)
> Created attachment 8816999 [details] [diff] [review]
> Proposed patch
> 
> Home page needs the same treatment.

Hi Khaled, did you mean to request review on this patch?

Thanks to both of you for looking at this! :-)

Comment 25

[Khaled Hosny]

(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> (In reply to Khaled Hosny from comment #21)
> > Created attachment 8816999 [details] [diff] [review]
> > Proposed patch
> > 
> > Home page needs the same treatment.
> 
> Hi Khaled, did you mean to request review on this patch?

I don’t really know what the process is, but yes these are for review.

Comment 26

[Khaled Hosny]

(In reply to Tomer Cohen :tomer from comment #23)
> (In reply to Khaled Hosny from comment #22)
> > > Nice idea. It is indeed fix this issue (reproduced with DevTools).
> > This is how I experimented with this, can’t wait until all Firefox UI is
> > HTML and hackable with DevTools :)
> Actually DevTools can be used to hack XUL pages as well.

Interesting. I was thinking about the search box in the address bar which is broken in a different way, can I use DevTools to hack it?

Comment 27

[Tomer Cohen :tomer]

(In reply to Khaled Hosny from comment #26)
> Interesting. I was thinking about the search box in the address bar which is
> broken in a different way, can I use DevTools to hack it?
The address for the main window is chrome://browser/content/browser.xul. 

If this bug is related to RTL, I'd very appreciate if you would CC me.

Comment 28

[Drew Willcoxon :adw]

Comment on attachment 8816999 [details] [diff] [review]
Proposed patch

Review of attachment 8816999 [details] [diff] [review]:
-----------------------------------------------------------------

Khaled, Tomer, very sorry for my delay in reviewing this.  Thanks very much for the patch, Khaled.

Comment 29

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d8dd19d01926144a32bf5d84ea61431864ea6a
Bug 1269186 - [RTL] Search field in New Tab and about:home is not RTL'd. r=adw

Comment 30

[Florian Quèze [:florian]]

This is not a recent regression, as the code this patch changes has been introduced in bug 744659 / Firefox 29, but given the patch is simple, I wonder if we should consider uplifting it to aurora / 52 so that our search access points all work correctly with RTL locales for the next ESR release.

Comment 31

[Iris Hsiao [:ihsiao]]

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

Comment 32

[Florian Quèze [:florian]]

Comment on attachment 8816999 [details] [diff] [review]
Proposed patch

Requesting the approval so that we remember to make a decision about uplifting to 52. If we uplift I would like QA to verify first though.

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression, this code has been here since Firefox 29.
[User impact if declined]: on RTL locales, text typed in the about:home and about:newtab searchboxes will be displayed with the wrong direction.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: See original bug description, and the screenshot attached in comment 3.
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: Not really.
[Why is the change risky/not risky?]: Small patch.
[String changes made/needed]: none

Comment 33

[Itiel]

(In reply to Florian Quèze [:florian] [:flo] from comment #32)
> Comment on attachment 8816999 [details] [diff] [review]
> Proposed patch
> 
> Requesting the approval so that we remember to make a decision about
> uplifting to 52. If we uplift I would like QA to verify first though.
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: Not a regression, this code has been
> here since Firefox 29.
> [User impact if declined]: on RTL locales, text typed in the about:home and
> about:newtab searchboxes will be displayed with the wrong direction.
> [Is this code covered by automated tests?]: No.
> [Has the fix been verified in Nightly?]: Not yet.
> [Needs manual test from QE? If yes, steps to reproduce]: See original bug
> description, and the screenshot attached in comment 3.
> [List of other uplifts needed for the feature/fix]: none.
> [Is the change risky?]: Not really.
> [Why is the change risky/not risky?]: Small patch.
> [String changes made/needed]: none

Well I can verify this is fixed on latest Nightly, but I'd say this patch possibly needs some adjustment.
After the patch, the "Search" text now appears on the right side, but the cursor is located on the left as default.
I think this should be changed so its location would be based on the directionality of the page.

Comment 34

[Itiel]

Attachment: Cursor LTR'd

Comment 35

[Tomer Cohen :tomer]

(In reply to ItielMaN from comment #33)
> Well I can verify this is fixed on latest Nightly, but I'd say this patch
> possibly needs some adjustment.
> After the patch, the "Search" text now appears on the right side, but the
> cursor is located on the left as default.
> I think this should be changed so its location would be based on the
> directionality of the page.

As far as I know, default caret location in dir=auto is undefined by the specs. I suggest that setting the default direction for undetermined content such as numbers or an empty string should be set by the parent element, but this is out of the scope of this issue and can't be done on the page level anyway.

Comment 36

[Julien Cristau [:jcristau]]

Setting qe-verify, as comment 32 suggests.

Comment 37

[roxleitan]

Verified as fixed using the latest Nightly 53 (Build ID:20170109030209) on Windows 10, Ubuntu 16.04 and Mac OS X 10.11

Comment 38

[Julien Cristau [:jcristau]]

Comment on attachment 8816999 [details] [diff] [review]
Proposed patch

RTL fix for about:home and about:newtab, aurora52+

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5425255d6615

Comment 40

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 46.0 (2016.04.21) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 52.0b1 and Firefox 53.0a2 on Win 8.1 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.3.

However, the behavior in the search box from "about:home" is not RTL. Is this behavior intended?
Link: http://imgur.com/a/6EAJZ .

Comment 41

[Itiel]

(In reply to Timea Zsoldos from comment #40)
> I have reproduced this issue using Firefox 46.0 (2016.04.21) on Win 8.1 x64.
> I can confirm this issue is fixed, I verified using Firefox 52.0b1 and
> Firefox 53.0a2 on Win 8.1 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.3.
> 
> However, the behavior in the search box from "about:home" is not RTL. Is
> this behavior intended?
> Link: http://imgur.com/a/6EAJZ .

Yes, I have noticed this a while ago.
I think there are actually 2 issues here, Tomer would say there's only one:
1. As default, the cursor is aligned to the left ,but it should be right-aligned. Tomer agrees with me here.
2. The cursor (regardless of it's default location) and the written text should behave as expected2 in attachment 8817617 [details].

Tomer disagrees with me about #2, and thinks it should behave as expected1.
I think this is wrong because it may confuse the user as to where's the cursor located when he starts typing. On a big enough screen, having a text that flips side everytime a user types characters in languages with opposite directionalities can be quite the annoyance.

Comment 42

[Itiel]

Tomer has convinced me that expected1 is the desired behaviour in #2 in comment 41, so the only issue left here is the default cursor alignment.

Comment 43

[Florian Quèze [:florian]]

(In reply to ItielMaN from comment #42)
> Tomer has convinced me that expected1 is the desired behaviour in #2 in
> comment 41, so the only issue left here is the default cursor alignment.

Please open new bugs for remaining issues, thanks!