Closed Bug 1204366 Opened 8 years ago Closed 6 years ago

[RTL] Searchbar's dropdown in wrong positions

Categories

(Firefox :: Search, defect, P2)

Product:
Firefox
Component:
Search
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: mvocom, Assigned: florian)

References

Details

(Keywords: rtl, Whiteboard: [fxsearch][RTL])

Attachments

(4 files, 2 obsolete files)

works for me.png
6.61 KB, image/png
Details
changed text direction.png
6.91 KB, image/png
Details
Patch v2
3.23 KB, patch
dao
: review+
Details | Diff | Splinter Review
Demo
173.54 KB, image/gif
Details 
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
Attached image works for me.png — 
Works for me. Can you reproduce this when using the default theme?
Flags: needinfo?(mvocom)
Component: Untriaged → Search
Keywords: rtl
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.
Flags: needinfo?(mvocom)
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
(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?
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.
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.
Thank you Gingerbread Man.

The problem also occurs in LTR UI if you change direction in the Search bar to RTL.
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P4
Whiteboard: [fxsearch]
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.
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.
(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.
Priority: P4 → P2
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.
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?
Summary: Searchbar's dropdown in wrong positions (in RTL) → Searchbar's dropdown in wrong positions
(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!
Summary: Searchbar's dropdown in wrong positions → [RTL] Searchbar's dropdown in wrong positions
> 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. :)
Rank: 22
Whiteboard: [fxsearch] → [fxsearch][RTL]
Attached patch WIP (Mac only) (obsolete) — — Splinter Review 
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.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — — Splinter Review 
Now with the Linux and Windows changes included.
Attachment #8758212 - Attachment is obsolete: true
Attachment #8758365 - Flags: review?(dao+bmo)
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...?
Attachment #8758365 - Flags: review?(dao+bmo)
(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.
(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?
Attached patch Patch v2 — — Splinter Review 
(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.
Attachment #8758365 - Attachment is obsolete: true
Attachment #8759152 - Flags: review?(dao+bmo)
Attachment #8759152 - Flags: review?(dao+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/4915ce29449e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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
QA Whiteboard: [bugday-20160803]
Flags: qe-verify+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I can still reprocude this issue under latest Nightly (53.0a1 21.1.17).
Version: 40 Branch → unspecified
(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?
Flags: needinfo?(itiel_yn8)
(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].
Flags: needinfo?(itiel_yn8)
Attached image Demo — 

    
    

    
  
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.)

    
    

    
  
(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.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
Resolution: --- → FIXED

    
  
Depends on: 1332962
See Also:  → 1332962

          You need to log in
          before you can comment on or make changes to this bug.