Closed Bug 813550 Opened 7 years ago Closed 7 years ago

Location bar's placeholder in an RTL chrome should be aligned on the right

Categories

(Firefox :: Address Bar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21
Tracking Status
firefox19 + verified
firefox20 --- verified
firefox21 --- verified

People

(Reporter: mounir, Assigned: dao)

References

Details

(Keywords: regression, rtl)

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #810796 +++

bug 737786 added some CSS property restrictions to ::-moz-placeholder that, it seems, are affecting the location bar placeholder direction with a RTL UI. For same reasons, the search bar placeholder is fine.
Summary: Fix invalid CSS in forms.css → Location bar's placeholder in an RTL chrome should be aligned on the right
Gavin, can you point me to the CSS rules that could be applied on the placeholder in the location bar? I wasn't able to find anything relevant :(
Flags: needinfo?(gavin.sharp)
(In reply to Mounir Lamouri (:mounir) from comment #2)
> Gavin, can you point me to the CSS rules that could be applied on the
> placeholder in the location bar?

I don't understand what you're asking for, but the location bar uses the uri-element-right-align class which used to take care of this.
(In reply to Dão Gottwald [:dao] from comment #3)
> (In reply to Mounir Lamouri (:mounir) from comment #2)
> > Gavin, can you point me to the CSS rules that could be applied on the
> > placeholder in the location bar?
> 
> I don't understand what you're asking for, but the location bar uses the
> uri-element-right-align class which used to take care of this.

But that doesn't apply to the placeholder, does it?

If I disable the ::-moz-placeholder CSS properties restrictions, things are working as expected so I think the issue is that something is applied to placeholder isn't working because it is restricted.
(In reply to Mounir Lamouri (:mounir) from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > (In reply to Mounir Lamouri (:mounir) from comment #2)
> > > Gavin, can you point me to the CSS rules that could be applied on the
> > > placeholder in the location bar?
> > 
> > I don't understand what you're asking for, but the location bar uses the
> > uri-element-right-align class which used to take care of this.
> 
> But that doesn't apply to the placeholder, does it?

It used to, apparently.
Keywords: regression
Flags: needinfo?(gavin.sharp)
Assignee: nobody → mounir
May be the removal in bug 810796 was not the right thing to do but it should have been a reordering of items?
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #6)
> May be the removal in bug 810796 was not the right thing to do but it should
> have been a reordering of items?

As said in the bug, bug 810796 had nothing to do with that bug. When I landed ::-moz-placeholder, I landed multiple patches and the one that made the regression in bug 810796 isn't the same as the one that made this one.
Bug 810796 is about a rule that wasn't correct. This one is about a property that seems to be ignored because of ::-moz-placeholder restrictions.

I need some help from the Firefox team to find out which property should be allowed to make this work and why the searchbar is working while the url bar isn't and if it would be possible to fix the url bar's placeholder to behave like the search bar one.
(In reply to Mounir Lamouri (:mounir) from comment #7)
> I need some help from the Firefox team to find out which property should be
> allowed to make this work and why the searchbar is working while the url bar
> isn't and if it would be possible to fix the url bar's placeholder to behave
> like the search bar one.

Unlike the searchbar that contains text from thrid parties, here is very little chance that the placeholder for urlbar would be in a non-RTL locale in a *real* RTL release.
(In reply to Mounir Lamouri (:mounir) from comment #7)
> I need some help from the Firefox team to find out which property should be
> allowed to make this work and why the searchbar is working while the url bar
> isn't and if it would be possible to fix the url bar's placeholder to behave
> like the search bar one.

As I said, as opposed to the search bar, the location bar uses the uri-element-right-align class. There's very little code backing that class: http://mxr.mozilla.org/mozilla-central/search?string=uri-element-right-align
Apparently "text-align: right" in forms.css used to take care of this but doesn't anymore.
There's also some rules that set "direction: ltr !important;". Maybe that's what's missing?
Mounir - have you made any progress here, or do we need to find a new assignee?
(In reply to Alex Keybl [:akeybl] from comment #11)
> Mounir - have you made any progress here, or do we need to find a new
> assignee?

I think someone from the Firefox team would be more appropriate.
Assignee: mounir → nobody
Jared - can you help find an assignee? Mounir just passed the puck after a month of being the assignee :)
Assignee: nobody → jaws
Is there any reason to keep the uri-element-right-align class?

I'd like to remove it anyways in bug 641238, and some basic testing shows removing it gets everything to look correctly.
We still need the writing direction to be left-to-right.
Alternatively, we could just add text-align:right; to uri-element-right-align.
(In reply to Jared Wein [:jaws] from comment #16)
> Alternatively, we could just add text-align:right; to
> uri-element-right-align.

http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#79 does this (only for non-placeholder text these days), bug 737786 tried to apply it to the placeholder, bug 810796 removed this since it wasn't working.
Attached patch patchSplinter Review
I'm not quite sure if this gets all edge cases right.

The uri-element-right-align being implemented across browser and layout code is confusing and inconvenient, so I moved everything to layout.
Attachment #703562 - Flags: review?(ehsan)
Assignee: jaws → dao
Status: NEW → ASSIGNED
Comment on attachment 703562 [details] [diff] [review]
patch

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

I tested this, and I think it covers all of the edge cases.  Thanks, Dao!
Attachment #703562 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/3bc030b6ee8f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment on attachment 703562 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 737786
User impact if declined: wrong alignment of the location bar's placeholder text ("Go to a Website") in RTL locales
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low. any regression risk is limited to the location bar in RTL locales
String or UUID changes made by this patch: none
Attachment #703562 - Flags: approval-mozilla-beta?
Attachment #703562 - Flags: approval-mozilla-aurora?
Comment on attachment 703562 [details] [diff] [review]
patch

Thanks Dao - approving for uplift.
Attachment #703562 - Flags: approval-mozilla-beta?
Attachment #703562 - Flags: approval-mozilla-beta+
Attachment #703562 - Flags: approval-mozilla-aurora?
Attachment #703562 - Flags: approval-mozilla-aurora+
Thank you Dao :)
How do you change the UI direction? With the Force RTL addon ?
I reproduced the issue from comment 1 on FF 19b3 with Force RTL addon.
Verified fixed on FF 19b4 Win7, Ubuntu 12.04, Mac OS X 10.7.5.

I also spotted something else while testing this: https://bugzilla.mozilla.org/show_bug.cgi?id=137995#c7
(In reply to comment #26)
> How do you change the UI direction? With the Force RTL addon ?

For future reference, the easiest way is to use Force RTL and open a new window after that or restart the browser (IIRC there used to be a few small edge cases that I never bothered to fix with the dynamic direction switch code.)
Verified fixed on FF 20RC Win7, Ubuntu 12.04, Mac OS X 10.7.5.
Verified fixed FF 21b7 Win7
You need to log in before you can comment on or make changes to this bug.