Closed Bug 412017 Opened 12 years ago Closed 12 years ago

[RTL][GTK] Native themed location bar (drop down button) looks bad on RTL layout

Categories

(Firefox :: Address Bar, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 beta3

People

(Reporter: twanno, Assigned: twanno)

References

Details

(Keywords: intl, rtl)

Attachments

(2 files, 2 obsolete files)

When bug 405210 gets fixed, the location bar drop down button will be themed natively on GTK. The text field has an open end towards the drop down button, so the drop down button looks attached to the text field. However on RTL layouts the drop down button will be positioned to the left of the location bar, and the text field itself will have an open end towards the right (into nothing) because its styled direction is LTR.

This patch fixes this by moving the styled LTR direction (which is needed for a correct text layout in the location bar) one child node further.
This causes the buttons and favicons in the location bar to switch place, I don't know if that is correct layout, but what gave me the idea that this might be the best solution is that the RTL Go button gets used, which at the moment is not the case.

There are however other solutions of which I will attach a screenshot.
Attachment #296631 - Flags: review?(mano)
Attached patch fix RTL layout of location bar (obsolete) — Splinter Review
Oops, that was on old version of the real patch.
Attachment #296631 - Attachment is obsolete: true
Attachment #296633 - Flags: review?(mano)
Attachment #296631 - Flags: review?(mano)
In this screen shot the top location bar is how it will look like on RTL layout when the patch for bug 405210 is commited.

Below that there are three options to fix the layout of the location bar in that case:
1) style the whole location bar LTR by default
2) style the drop down button and the text field border RTL, but let the content be layed out LTR.
3) my solution: only style the input field LTR and the rest RTL.

The current situation, without the patch for bug 405210, is that of option 2.
Comment on attachment 296633 [details] [diff] [review]
fix RTL layout of location bar

r=mano, Thanks!
Attachment #296633 - Flags: review?(mano) → review+
> But please fix winstripe too...
Not sure if I should request for review again, but here you go!
Attachment #296633 - Attachment is obsolete: true
Attachment #296643 - Flags: review?(mano)
Comment on attachment 296643 [details] [diff] [review]
fix RTL layout of location bar (including winstripe).

Fix RTL issue with location bar on gtk (but also an issue on winstripe).
Attachment #296643 - Flags: approval1.9?
Comment on attachment 296643 [details] [diff] [review]
fix RTL layout of location bar (including winstripe).

I think this is wrong. The size and number of urlbar icons varies. So this will shift the address.
Attachment #296643 - Flags: review-
Comment on attachment 296631 [details] [diff] [review]
fix RTL layout of location bar

(In reply to comment #7)
> (From update of attachment 296643 [details] [diff] [review])
> I think this is wrong. The size and number of urlbar icons varies. So this will
> shift the address.
> 
Then the version I had in mind first might be the best solution (the second option mentioned in comment #2, the third in the screenshot)
Attachment #296631 - Attachment is obsolete: false
Attachment #296631 - Flags: review?(dao)
Attachment #296643 - Attachment is obsolete: true
Attachment #296643 - Flags: approval1.9?
Attachment #296631 - Flags: review?(mano)
(In reply to comment #8)
> Then the version I had in mind first might be the best solution (the second
> option mentioned in comment #2, the third in the screenshot)

In the third picture, there appears to be a margin or padding between the dropmarker and the site button (the box with the favicon). Could you please take a look at where this is coming from?

Also, the arrow within the dropmarker isn't centered. Is this a bug in the widget code?
(In reply to comment #9)
> (In reply to comment #8)
> > Then the version I had in mind first might be the best solution (the second
> > option mentioned in comment #2, the third in the screenshot)
> 
> In the third picture, there appears to be a margin or padding between the
> dropmarker and the site button (the box with the favicon). Could you please
> take a look at where this is coming from?
> 
This is because the open border of the entry widget is created by the GTK theme. It replaces the normal border with what the theme assumes must be the text field background color (white). There is AFAIK nothing we can do about that. 

> Also, the arrow within the dropmarker isn't centered. Is this a bug in the
> widget code?
> 
That could be, I will look into that.
(In reply to comment #10)
> This is because the open border of the entry widget is created by the GTK
> theme. It replaces the normal border with what the theme assumes must be the
> text field background color (white). There is AFAIK nothing we can do about
> that.

Does it assume the wrong background even in LTR mode if you're on a secure site (i.e. yellow urlbar)?
(In reply to comment #11)
> Does it assume the wrong background even in LTR mode if you're on a secure site
> (i.e. yellow urlbar)?
> 
Yes, it does :(
So you really see no way to work around this? Drawbacks like this spoil the native theming, and I think it's bad enough to block bug 405210. I don't like the current dropmaker for various reasons, but it could be improved without native theming.
This can be fixed with the following style rule (and current patch):

>#urlbar > .autocomplete-textbox-container {
>  -moz-margin-end: -2px;
>}

Shall I include this in the current patch, or should that be fixed in bug 405210?

Also I found the arrow centering is a bug in the widget code. So I guess a new patch is required there anyway.
(In reply to comment #14)
> This can be fixed with the following style rule (and current patch):
> 
> >#urlbar > .autocomplete-textbox-container {
> >  -moz-margin-end: -2px;
> >}
> 
> Shall I include this in the current patch, or should that be fixed in bug
> 405210?

This belongs to bug 405210, and please ask a toolkit peer for review.
Attachment #296631 - Flags: review?(dao) → review+
Comment on attachment 296631 [details] [diff] [review]
fix RTL layout of location bar

and winstripe?
Attachment #296631 - Flags: review?(mano) → review+
winstripe doesn't have the problem described in comment 0 (due to bug 405210).
Attachment #296631 - Flags: approval1.9?
Attachment #296631 - Flags: approval1.9? → approval1.9+
Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.154; previous revision: 1.153
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M11
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
You need to log in before you can comment on or make changes to this bug.