Closed Bug 1206709 Opened 9 years ago Closed 7 years ago

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

Categories

(Firefox :: Search, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox50 --- verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: mvocom, Assigned: florian, Mentored)

References

Details

(Keywords: rtl, Whiteboard: [fxsearch][good first bug][lang=css])

Attachments

(2 files)

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
Component: Untriaged → Search
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: rtl
Priority: -- → P3
Summary: New Search UI: Missing and Superfluous separators → [RTL] mispositioned separators between one-off buttons in the searchbar panel
Whiteboard: [fxsearch]
Thanks Florian.
QA Whiteboard: community@rtl.bugs, smontagu@smontagu.org,
QA Whiteboard: community@rtl.bugs, smontagu@smontagu.org,
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
Mentor: ntim.bugs
Whiteboard: [fxsearch] → [fxsearch][good first bug][lang=css]
(In reply to Tim Nguyen [:ntim] from comment #2)

> contentSearchUI.css

Are you sure this file applies to the findbar in the toolbar?
Rank: 35
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;
}
Thanks for looking into this. Would you be able to attach your solution in the form of a patch?
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.
I have never done anything like a patch (that I am aware).  What is a patch?  Maybe I can do it!
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? :)
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? :)
Assignee: nobody → sahotaj7
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.
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
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
Thank you all for working on this issue.

@Joshpal,
The problem is in RTL locals.

***
Could anyone have a look at Bug 1204366?
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
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. :)
(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.
Blocks: 1171344
Attached patch PatchSplinter Review
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.
Attachment #8757984 - Flags: review?(adw)
Assignee: sahotaj7 → florian
Status: NEW → ASSIGNED
Thank you Florian.
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
(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.
(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.
Attachment #8757984 - Flags: review?(adw) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/ec2adf340418
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Flags: qe-verify+
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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.
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Version: 40 Branch → unspecified
(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.
(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.
Status: REOPENED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: