1206709 - [RTL] mispositioned separators between one-off buttons in the searchbar panel

Status: VERIFIED FIXED

(Firefox :: Search, defect, P3)

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:

There should be vertical separators on both sides of the Wikipedia icon, and one only on the left side of the Yahoo icon.

http://s13.postimg.org/fqobxdarb/FFSearch.png

Comment 1

[Yaron]

Thanks Florian.

Comment 2

[Tim Nguyen :ntim]

At [0], you should replace the background rule with:

background: none;
border-inline-end: 1px solid;
border-image: linear-gradient(transparent 15%,rgba(0, 0, 0, 0.08) 15%,rgba(0, 0, 0, 0.08) 85%, transparent 85%) 1;

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/contentSearchUI.css?offset=0#101

Comment 3

[Florian Quèze [:florian]]

(In reply to Tim Nguyen [:ntim] from comment #2)

> contentSearchUI.css

Are you sure this file applies to the findbar in the toolbar?

Comment 4

[johngraciliano]

I was about to report this but:
The separator between search engines in the search results table is misplaced for RTL locales.

I believe this problem has been there since the adoption of the new searchbar.

The correction for about:home and about:newtab needs the following rule to be inserted into chrome://browser/content/contentSearchUI.css

.contentSearchOneOffItem:-moz-dir(rtl) {
  background-position: left center;
}

For the search results in the searchbar in the browser main interface the next rule has to be inserted into chrome://browser/skin/searchbar.css

.searchbar-engine-one-off-item:-moz-locale-dir(rtl){
  background-position: left center;
}


p.s. There is a rule-set in chrome://browser/content/contentSearchUI.css:

.contentSearchHeaderRow > td > img,
.contentSearchSuggestionRow > td > .historyIcon {
  margin-right: 8px;
  margin-bottom: -3px;
}

for the correct layout in RTL locales, it should be replaced by:

.contentSearchHeaderRow > td > img,
.contentSearchSuggestionRow > td > .historyIcon {
  -moz-margin-end: 8px;
  margin-bottom: -3px;
}

Comment 5

[johngraciliano]

You Must Apply My Solution Immediatelly!
OBEY!

Comment 6

[johngraciliano]

My apologies to all. You should consider my solution for a prompt solution to this long standing problem.
Please.

Comment 7

[Florian Quèze [:florian]]

Thanks for looking into this. Would you be able to attach your solution in the form of a patch?

Comment 8

[johngraciliano]

p.p.s.  One more thing.  You also need to add into chrome://browser/content/contentSearchUI.css:

.contentSearchSuggestionTable:-moz-dir(rtl){
  left: auto;
  right: 0;
}

So that the placement of the panel is right aligned and extendes to the left on RTL locales.

Comment 9

[johngraciliano]

I have never done anything like a patch (that I am aware).  What is a patch?  Maybe I can do it!

Comment 10

[Florian Quèze [:florian]]

There are some explanations at https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Let me know if you need more help.

Comment 11

[Joshpal Sahota]

Hi, i'd like to work on this bug as my first bug. Im new to HTML and CSS and i thought this would be a good bug to start with :D

Is that okay? :)

Comment 12

[Joshpal Sahota]

Hi, i'd like to work on this bug as my first bug. Im new to HTML and CSS and i thought this would be a good bug to start with :D

Is that okay? :)

Comment 13

[johngraciliano]

Joshpal:
Check out my comment #4 and comment #8.  I am fairly sure they would work, but I was not able to propose my solution in the form of a patch.  If you find my suggestion is fine, maybe you can put have it go properly as a patch.
Regardless, thanks taking over the task of solving this.
--johnGraciliano
p.s.  I never tried Tim's proposal in comment #2.  It may lead to a better solution than what I did.

Comment 14

[Joshpal Sahota]

Hi John, thanks for replying to my message. Do I simply create a new file for the patch or do i update the current .css file with the code from comment #4 and comment #8?

In the current .css file I can't see where im supposed to change the code for the separators :S

Comment 15

[Joshpal Sahota]

I have tried to view the mispositioned separators in Firefox however it seems to be fine. Please see image: http://s16.postimg.org/948txlw8l/Separators.jpg

Comment 16

[Yaron]

Thank you all for working on this issue.

@Joshpal,
The problem is in RTL locals.

***
Could anyone have a look at Bug 1204366?

Comment 17

[Joshpal Sahota]

Hi Yaron, as i've showed in the image: http://s16.postimg.org/948txlw8l/Separators.jpg

The bug already seems to be fixed so im not quite sure what im fixing

Thanks for the help

Comment 18

[Yaron]

Hi Joshpal,

Please install the Hebrew (IL) Language Pack.
https://addons.mozilla.org/en-US/firefox/addon/hebrew-il-language-pack/
(For FF 42).

In about:config set general.useragent.locale to he.

Restart FF.

Now, your layout should be RTL.
http://s13.postimg.org/fqobxdarb/FFSearch.png

The code johngraciliano kindly posted in comment #4 and comment #8 solves the problem.

I've never created a patch.

Thank you for your work and best of luck. :)

Comment 19

[Tim Nguyen :ntim]

(In reply to Joshpal Sahota from comment #17)
> Hi Yaron, as i've showed in the image:
> http://s16.postimg.org/948txlw8l/Separators.jpg
> 
> The bug already seems to be fixed so im not quite sure what im fixing
> 
> Thanks for the help

You need an RTL locale. You can use https://addons.mozilla.org/en-US/firefox/addon/force-rtl/ . Then check "Force RTL Direction" from the menu if you're not seeing any change.

Comment 21

[Florian Quèze [:florian]]

Attachment: Patch

I took the suggestions from comment 4 and comment 8, and that worked.

I added some padding fixes in newTab.css and aboutHome.css to avoid having the search term under the glass icon in the input fields of about:home/about:newtab.

Comment 22

[Yaron]

Thank you Florian.

Comment 23

[Tim Nguyen :ntim]

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

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

::: browser/base/content/contentSearchUI.css
@@ +110,4 @@
>    padding: 0;
>    border: none;
>    background: none;
>    background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAAWCAYAAAABxvaqAAAABmJLR0QA/wD/AP+gvaeTAAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH3gofECQNNVW2/AAAABBJREFUGFdjOHPmzH8GehEA/KpKg9YTf4AAAAAASUVORK5CYII=');

Using a border-image would simply this code a lot.

border-inline-end: 1px solid;
border-image: linear-gradient(transparent 15%,rgba(0, 0, 0, 0.08) 15%,rgba(0, 0, 0, 0.08) 85%, transparent 85%) 1;

That also means you can omit the RTL specific background-position rule.

@@ +114,5 @@
>    background-repeat: no-repeat;
>    background-position: right center;
>  }
>  
> +.contentSearchOneOffItem:-moz-dir(rtl) {

nit: :dir(rtl) is now supported

Comment 24

[Florian Quèze [:florian]]

(In reply to Tim Nguyen :ntim from comment #23)
> Comment on attachment 8757984 [details] [diff] [review]
> Patch
> 
> Review of attachment 8757984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/contentSearchUI.css
> @@ +110,4 @@
> >    padding: 0;
> >    border: none;
> >    background: none;
> >    background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAAWCAYAAAABxvaqAAAABmJLR0QA/wD/AP+gvaeTAAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH3gofECQNNVW2/AAAABBJREFUGFdjOHPmzH8GehEA/KpKg9YTf4AAAAAASUVORK5CYII=');
> 
> Using a border-image would simply this code a lot.

Wouldn't adding a border require changes to the way sizes are calculated in the JS code? I agree it would make the code cleaner (and I dislike this base64 encoded image in the CSS file), but I would prefer this to be thought through in a separate cleanup bug.

Comment 25

[Tim Nguyen :ntim]

(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> (In reply to Tim Nguyen :ntim from comment #23)
> > Comment on attachment 8757984 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 8757984 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/base/content/contentSearchUI.css
> > @@ +110,4 @@
> > >    padding: 0;
> > >    border: none;
> > >    background: none;
> > >    background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAAWCAYAAAABxvaqAAAABmJLR0QA/wD/AP+gvaeTAAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH3gofECQNNVW2/AAAABBJREFUGFdjOHPmzH8GehEA/KpKg9YTf4AAAAAASUVORK5CYII=');
> > 
> > Using a border-image would simply this code a lot.
> 
> Wouldn't adding a border require changes to the way sizes are calculated in
> the JS code? I agree it would make the code cleaner (and I dislike this
> base64 encoded image in the CSS file), but I would prefer this to be thought
> through in a separate cleanup bug.
If the element has box-sizing: border-box; adding a border should have no effect on how the size is calculated. Also, you can already get rid of the data URI without using border-image by using a gradient, and by tweaking the bg-position/bg-repeat.

Comment 26

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/ec2adf340418aafb963aa470af00275918b8223e
Bug 1206709 - [RTL] mispositioned separators between one-off buttons in the searchbar panel, original patch by johngraciliano, r=adw.

Comment 27

[Carsten Book [:Tomcat]]

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

Comment 28

[Ciprian Georgiu, Desktop QA]

This issue is verified fixed on Firefox Beta 50.0b6-build1 (2016-10-10) with a RTL locale "he", on the following OSes:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- Mac OS X 10.11 Beta

Comment 29

[Ciprian Georgiu, Desktop QA]

Also, the issue is no longer reproducible on Firefox 51.0a2 (2016-10-12) and Firefox 52.0a1 (2016-10-11), builds localized in Arabic, Persian and Hebrew.
Tests were performed under Windows 10 x64, Mac OS X 10.11.6, Ubuntu 16.04 x64.

Comment 30

[Itiel]

Sorry for bumping in here.
It seems as if changing Windows DPI scaling to more than 125% (in my case I'm running 1920x1080 resolution on a 24" monitor), the bug reappears.

See attached.

Comment 31

[Itiel]

Attachment: Issue reappearing on 150% DPI scaling

Comment 32

[Itiel]

(In reply to ItielMaN from comment #30)
> Sorry for bumping in here.
> It seems as if changing Windows DPI scaling to more than 125% (in my case
> I'm running 1920x1080 resolution on a 24" monitor), the bug reappears.
> 
> See attached.

Though, I'm not sure if this issue is related to this bug.

See also bug 1325816.

Comment 33

[Florian Quèze [:florian]]

(In reply to ItielMaN from comment #30)
> Sorry for bumping in here.

Next time please file a new bug instead of reopening a bug that been fixed a couple releases ago, and already VERIFIED as FIXED by QA.

The issues you reported (and shown on your screenshot) is bug 1104325, it's related to Windows HiDPI, but isn't specific to RTL.