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

VERIFIED FIXED in Firefox 52

Status

()

Firefox
New Tab Page
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: Itiel, Assigned: Khaled Hosny)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified, firefox53 verified)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8747507 [details]
Search button should point left.png
(Reporter)

Comment 2

2 years ago
Created attachment 8747508 [details]
Letters hidden by the magnifying glass.png
(Reporter)

Comment 3

2 years ago
Created attachment 8747509 [details]
Search word should be RTL'd.png
(Reporter)

Comment 4

2 years ago
Forgot to mention, I'm using Windows 10. Though I don't think this changes across different platforms.
Component: Untriaged → New Tab Page
(Reporter)

Comment 5

2 years ago
Needless to say, this also applies to about:home page.
(Reporter)

Comment 6

2 years ago
Same issue in the latest build of Develpoer Edition

Comment 7

2 years ago
Is it the same case with old versions of Firefox like 45 or 40?
(Reporter)

Comment 8

2 years ago
(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

2 years ago
Ok, so it doesn't sound like a recent regression.
(Reporter)

Updated

2 years ago
Summary: Search field in New Tab is not RTL'd in RTL languages → Search field in New Tab and about:home is not RTL'd in RTL languages
(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.
(Reporter)

Comment 11

2 years ago
(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 :)
(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.
(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.
(Reporter)

Comment 14

2 years ago
(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)
(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.
(Reporter)

Comment 16

2 years ago
#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.
(Reporter)

Updated

2 years ago
Blocks: 137995
Summary: Search field in New Tab and about:home is not RTL'd in RTL languages → [RTL] Search field in New Tab and about:home is not RTL'd
Version: 46 Branch → unspecified
(Reporter)

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 17

2 years ago
(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.
(Assignee)

Comment 18

2 years ago
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.
(Assignee)

Comment 19

2 years ago
Created attachment 8816996 [details] [diff] [review]
Proposed patch

Comment 20

2 years ago
Nice idea. It is indeed fix this issue (reproduced with DevTools).
(Assignee)

Comment 21

2 years ago
Created attachment 8816999 [details] [diff] [review]
Proposed patch

Home page needs the same treatment.
Attachment #8816996 - Attachment is obsolete: true
(Assignee)

Comment 22

2 years ago
(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

2 years ago
(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.
(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! :-)
Flags: needinfo?(khaledhosny)
(Assignee)

Comment 25

2 years ago
(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.
Flags: needinfo?(khaledhosny)
(Assignee)

Comment 26

2 years ago
(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

2 years ago
(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.

Updated

2 years ago
See Also: → bug 744659

Updated

2 years ago
Attachment #8816999 - Flags: review?(adw)

Comment 28

2 years ago
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.
Attachment #8816999 - Flags: review?(adw) → review+

Updated

2 years ago
Assignee: nobody → khaledhosny
Status: NEW → ASSIGNED
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d1d8dd19d019
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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
Attachment #8816999 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 33

2 years ago
(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.
(Reporter)

Comment 34

2 years ago
Created attachment 8824698 [details]
Cursor LTR'd

Comment 35

2 years ago
(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.
Setting qe-verify, as comment 32 suggests.
Flags: qe-verify+
Verified as fixed using the latest Nightly 53 (Build ID:20170109030209) on Windows 10, Ubuntu 16.04 and Mac OS X 10.11
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Comment on attachment 8816999 [details] [diff] [review]
Proposed patch

RTL fix for about:home and about:newtab, aurora52+
Attachment #8816999 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 39

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/5425255d6615
status-firefox52: --- → fixed
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 .
status-firefox52: fixed → verified
Flags: qe-verify+ → needinfo?(itiel_yn8)
(Reporter)

Comment 41

a year ago
(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.
Flags: needinfo?(itiel_yn8) → needinfo?(tomer.moz.bugs)
(Reporter)

Comment 42

a year ago
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.
Flags: needinfo?(tomer.moz.bugs)
(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!
(Reporter)

Updated

a year ago
Depends on: 1335177
You need to log in before you can comment on or make changes to this bug.