Closed
Bug 1206709
Opened 9 years ago
Closed 8 years ago
[RTL] mispositioned separators between one-off buttons in the searchbar panel
Categories
(Firefox :: Search, defect, P3)
Firefox
Search
Tracking
()
VERIFIED
FIXED
Firefox 50
People
(Reporter: mvocom, Assigned: florian, Mentored)
References
Details
(Keywords: rtl, Whiteboard: [fxsearch][good first bug][lang=css])
Attachments
(2 files)
(deleted),
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details |
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
Assignee | ||
Updated•9 years ago
|
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]
Comment 2•9 years ago
|
||
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]
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Tim Nguyen [:ntim] from comment #2)
> contentSearchUI.css
Are you sure this file applies to the findbar in the toolbar?
Updated•9 years ago
|
Rank: 35
Comment 4•9 years ago
|
||
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 hidden (typo) |
Comment hidden (obsolete) |
Assignee | ||
Comment 7•9 years ago
|
||
Thanks for looking into this. Would you be able to attach your solution in the form of a patch?
Comment 8•9 years ago
|
||
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•9 years ago
|
||
I have never done anything like a patch (that I am aware). What is a patch? Maybe I can do it!
Assignee | ||
Comment 10•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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? :)
Updated•9 years ago
|
Assignee: nobody → sahotaj7
Comment 13•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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
Reporter | ||
Comment 16•9 years ago
|
||
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•9 years ago
|
||
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
Reporter | ||
Comment 18•9 years ago
|
||
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•9 years ago
|
||
(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.
Assignee | ||
Comment 21•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: sahotaj7 → florian
Status: NEW → ASSIGNED
Reporter | ||
Comment 22•8 years ago
|
||
Thank you Florian.
Comment 23•8 years ago
|
||
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
Assignee | ||
Comment 24•8 years ago
|
||
(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•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8757984 -
Flags: review?(adw) → review+
Assignee | ||
Comment 26•8 years ago
|
||
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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•8 years ago
|
Flags: qe-verify+
Comment 28•8 years ago
|
||
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•8 years ago
|
||
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.
status-firefox51:
--- → verified
status-firefox52:
--- → verified
Comment 30•8 years ago
|
||
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
Comment 31•8 years ago
|
||
Comment 32•8 years ago
|
||
(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.
Assignee | ||
Comment 33•8 years ago
|
||
(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 ago → 8 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•