1204366 - [RTL] Searchbar's dropdown in wrong positions

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Yaron]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150826023504

Steps to reproduce:

In RTL locals (Hebrew etc.), the Searchbar's dropdown is not aligned properly.

http://s3.postimg.org/xzplttnn7/FF_En.png
http://s4.postimg.org/flx4tmjd9/FF_He.png

Comment 1

[Gingerbread Man]

Attachment: works for me.png

Works for me. Can you reproduce this when using the default theme?

Comment 2

[Yaron]

Change text direction (Ctrl+Shift+X).
http://s14.postimg.org/qjpyb3qht/FF_He_Default_Theme.png

Searching mostly in English, I've added "#search-container .textbox-input-box { direction: ltr !important; }" to userChrome.css.

The problem exists in the Old Search UI (much better IMHO) as well, but not to the same degree.

I think the code in search.xml needs to be modified.
const isRTL = getComputedStyle(this, "").direction == "rtl";
...

Thank you.

Comment 3

[Yaron]

Note also the difference between opening the dropdown with a value and without one.

http://s22.postimg.org/bfd35kkkx/Value.png
http://s28.postimg.org/6qyz9bxq5/No_Value.png

Comment 4

[Florian Quèze [:florian]]

(In reply to Yaron from comment #2)
> Change text direction (Ctrl+Shift+X).
> http://s14.postimg.org/qjpyb3qht/FF_He_Default_Theme.png
> 
> Searching mostly in English, I've added "#search-container
> .textbox-input-box { direction: ltr !important; }" to userChrome.css.

Is the problem only happening when you change something in userChrome.css?

Comment 5

[Yaron]

The problem occurs when you change text direction to LTR (regardless of userChrome.css).
I've added that line to save myself changing the direction manually. 

I assume your UI is LTR.
Could you please change text direction in the Search bar and check how the dropdown is displayed?
(I forgot to do that when I changed my UI). 

Thank you.

Comment 6

[Gingerbread Man]

Attachment: changed text direction.png

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0
20150914030205
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
20150826023504

(In reply to Yaron from comment #2)
> Change text direction (Ctrl+Shift+X).

In that case, I can reproduce the issue.

(In reply to Yaron from comment #5)
> I assume your UI is LTR.

I tested the Hebrew versions of Firefox 40.0.3 and the latest Nightly, in a brand new profile.

Comment 7

[Yaron]

Thank you Gingerbread Man.

The problem also occurs in LTR UI if you change direction in the Search bar to RTL.

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8660550 [details]
works for me.png

This screenshot shows another problem. There should be vertical bars on both sides of the wikipedia icon, and there should be one only on one side of the Yahoo icon.

Comment 9

[Yaron]

Hi Florian,

Nice observation. :)

Can you take this bug (the main issue)?
If not, could you please think of someone else who would?

I'd like to emphasize that - as I've mentioned earlier - this bug is relevant to LTR UI as well (i.e. if you change direction in the Search bar to RTL).

I think this is a major issue.
Would it be possible to keep the old Search UI as long as it's not fixed? 

***

search.xml:
popup.openPopup(this.inputField, "after_start", 0, yOffset, false, false);

If you replace "after_start" with "after_end", "direction: ltr" - displayed properly and vice versa (checked in RTL UI; I assume it would be the other way around in LTR UI).

Thank you.

Comment 10

[Florian Quèze [:florian]]

(In reply to Yaron from comment #9)

> Can you take this bug (the main issue)?

I tried for a few minutes yesterday, but couldn't find an obvious fix.

> If not, could you please think of someone else who would?

I don't know right now. I'll try to think about it.

> Would it be possible to keep the old Search UI as long as it's not fixed?

The old search UI is already removed, so no.


> ***
> 
> search.xml:
> popup.openPopup(this.inputField, "after_start", 0, yOffset, false, false);
> 
> If you replace "after_start" with "after_end", "direction: ltr" - displayed
> properly and vice versa (checked in RTL UI; I assume it would be the other
> way around in LTR UI).

Is this a proposed fix, or a way to reproduce? I'm confused.

Comment 11

[Yaron]

Thank you Florian. I appreciate it.

If you replace "after_start" with "after_end"...
In my RTL UI, if the current direction is in the Search bar is LTR - the popup is displayed properly; - if the current direction is RTL, the popup is NOT displayed properly (the behavior is reversed).

So, It might be a direction to a fix.
- Theoretically, if you check the *current* direction before opening the popup and then accordingly use either "after_start" or "after_end", it might fix the issue. 

Another possible direction is:
popup.openPopup(null, "after_start", ???, outerRect.bottom, false, false);
x position - I don't know.
"after_start" is ignored.

Regards.

Comment 12

[Yaron]

Another issue:

The "Search with current engine" segment is a command, and should have a different background for hover and hover:active.

http://s2.postimg.org/imqbhdkux/Search_With_Current.png

And shouldn't "Search settings" have a different background for hover:active?

***
Should I file a separate bug?

Comment 13

[Florian Quèze [:florian]]

(In reply to Yaron from comment #12)
> Another issue:
> 
> The "Search with current engine" segment is a command, and should have a
> different background for hover and hover:active.
> 
> http://s2.postimg.org/imqbhdkux/Search_With_Current.png
> 
> And shouldn't "Search settings" have a different background for hover:active?
> 
> ***
> Should I file a separate bug?

These things are both unrelated to this bug, so yes, if you think something needs to be fixed/improved, please file a separate bug for each issue. Thanks!

Comment 14

[Yaron]

> Another possible direction is:
> popup.openPopup(null, "after_start", ???, outerRect.bottom, false, false);
> x position - I don't know.

popup.openPopup(null, -1, -outerRect.left, outerRect.bottom, false, false);

Solves the problem in RTL locals.

Please don't quit working on a better solution. :)

Comment 15

[Florian Quèze [:florian]]

Attachment: WIP (Mac only)

We are forcing the panel to have a width larger than the textfield's width using JS code. The alignment between the panel and the searchbox is obtained using a negative margin in searchbar.css. When the text field is in the normal direction (ie. same direction as the rest of the UI), margin-inline-start is used. When the textfield's direction has been reversed (eg. using Command+shift+X), we need to set margin-inline-end instead.

I assume this value will need to be different for each of the platforms, and I've only tested on Mac for now, so this WIP patch only changes the Mac CSS.

Comment 16

[Florian Quèze [:florian]]

Attachment: Patch

Now with the Linux and Windows changes included.

Comment 17

[Dão Gottwald [:dao]]

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

(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> We are forcing the panel to have a width larger than the textfield's width
> using JS code. The alignment between the panel and the searchbox is obtained
> using a negative margin in searchbar.css. When the text field is in the
> normal direction (ie. same direction as the rest of the UI),
> margin-inline-start is used. When the textfield's direction has been
> reversed (eg. using Command+shift+X), we need to set margin-inline-end
> instead.

Can you please add something along those lines as a documenting comment to the relevant CSS rule?

Also please double-check the numbers. I only tested on Linux where your margin-line-end appears to be off by one pixel...?

Comment 18

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [:dao] from comment #17)

> Also please double-check the numbers. I only tested on Linux where your
> margin-line-end appears to be off by one pixel...?

The whole panel is already off by one pixel on Linux for me. I purposefully kept it this way for the margin-line-end, so that the panel doesn't move at all when pressing Ctrl+shift+X. If you want to fix this off-by-one pixel issue, we'll need to change the margin-inline-start value too. And I think we set it this way to preserve the alignment between the glass icon and the search engine's icon.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> (In reply to Dão Gottwald [:dao] from comment #17)
> 
> > Also please double-check the numbers. I only tested on Linux where your
> > margin-line-end appears to be off by one pixel...?
> 
> The whole panel is already off by one pixel on Linux for me. I purposefully
> kept it this way for the margin-line-end, so that the panel doesn't move at
> all when pressing Ctrl+shift+X. If you want to fix this off-by-one pixel
> issue, we'll need to change the margin-inline-start value too.

Please feel free to do that.

> And I think
> we set it this way to preserve the alignment between the glass icon and the
> search engine's icon.

This doesn't sound like the right trade-off to me. Why would we do this only on Linux anyway?

Comment 20

[Florian Quèze [:florian]]

Attachment: Patch v2

(In reply to Dão Gottwald [:dao] from comment #17)

> Also please double-check the numbers. I only tested on Linux where your
> margin-line-end appears to be off by one pixel...?

I only changed the margin-inline-start by 1px on Linux. On Windows the alignment looks correct on my machine. On Mac it's slightly off, but by less than 1px (there must be a rounding issue somewhere), and both the searchbar and the panel have rounded corners and shadows/large focus highlights, so it's not really visible.

Comment 21

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/4915ce29449ef3595957114ecdff8d036b72f238
Bug 1204366 - [RTL] Searchbar's dropdown in wrong positions, r=dao.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4915ce29449e

Comment 23

[Saheda Reza [:Antora]]

I have reproduced this bug on firefox nightly according to()

The fixing bug has verified on Latest Nightly -- Build ID:(20160805004022),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0

Tested OS--Windows10 32bit

Comment 24

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 39.0 (HE) build, under Windows 10 x64.

The issue was partially fixed, but the position of the Searchbar's dropdown it's not kept if changing the text direction in the Search-bar (Ctrl+Shift+x). The Searchbar's dropdown is moving 1px to Right/Left, according to the text direction.
The issue is reproducible on Firefox 50.0b6 (he)(fa)(ar)(en-US) builds, Firefox 52.0a1 (2016-10-10) and on Windows 10, x64, Mac OS X 10.11.6 and on Ubuntu 16.04 x64
Note that the issue is reproducible on Firefox 51.0a2 (2016-10-10) only on Ubuntu OS.

Here is a screencast of the issue: https://goo.gl/wwYavj

Since the position of the Searchbar's dropdown is not kept, I am reopening this issue.

Comment 25

[Itiel]

I can still reprocude this issue under latest Nightly (53.0a1 21.1.17).

Comment 26

[Florian Quèze [:florian]]

(In reply to ItielMaN from comment #25)
> I can still reprocude this issue under latest Nightly (53.0a1 21.1.17).

Which issue can you still reproduce? Is it the 1px move described in comment 24 (if so please open a new bug for it), or the original issue?

Comment 27

[Itiel]

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> Which issue can you still reproduce? Is it the 1px move described in comment
> 24 (if so please open a new bug for it), or the original issue?

Well actually I can reproduce both.
See attached GIF, demonstrating the 1px difference and the issue shown in attachment 8661049 [details].

Comment 28

[Itiel]

Attachment: Demo

Comment 29

[Florian Quèze [:florian]]

On your gif it looks like you can reproduce when clicking the glass icon, but not when typing and getting suggestions. Is that right? (If so, please file a new bug; we are not going to fix additional edge cases in this bug that has been resolved months ago.)

Comment 30

[Itiel]

(In reply to Florian Quèze [:florian] [:flo] from comment #29)
> On your gif it looks like you can reproduce when clicking the glass icon,
> but not when typing and getting suggestions. Is that right? (If so, please
> file a new bug; we are not going to fix additional edge cases in this bug
> that has been resolved months ago.)

Yep, that's right, thanks for pointing that out.
Will open a another bug for it.