810796 - Fix invalid CSS in forms.css

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mounir Lamouri (:mounir)]

https://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#90 is doing "::-moz-placeholder[dir=ltr]" which is invalid.

I believe this is there to make text field with an explicit ltr direction to have their placeholder aligned on the left even if the UI is rtl.

I'm not sure how we could fix this and keep the behaviour.

Comment 1

[Boris Zbarsky [:bzbarsky]]

What set the dir=ltr attribute on the placeholder div before, if anything?  I don't see anything obvious, so looks like this rule was dead before too.  If so, removing it sounds just fine.  ;)

Comment 3

[Alex Keybl [:akeybl]]

While we understand the CSS is invalid, it's not clear what the user impact is here if left unfixed.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Chrome UI in RTL before placeholder changes

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Chrome UI in RTL after placeholder changes

Comment 6

[Mounir Lamouri (:mounir)]

See how after placeholder changes, the placeholder is misplaced: it was on the right and is now on the left.

mozregression list those commits as suspects:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90cea19e27e2&tochange=a47525b93528

I could bisect on m-c to check which exact commit is responsible but I would say that we have somehow an issue with RTL chrome UI here (whether this is the mis-formed line or another).

Comment 7

[Mounir Lamouri (:mounir)]

(CCing some RTL folks)
I am assuming that the behaviour we have in Nightly is incorrect, right? By any chance, do you see what's wrong?

Comment 8

[(no longer active)]

Yeah I think the text should be left-aligned if the textbox is LTR.

Comment 9

[Simon Montagu :smontagu]

I'm not following: the line with ::-moz-placeholder was only added just now, in bug 737786. I believe that adding it wasn't necessary and it can just go (though I haven't tested that doing that fixes the regression in alignment)

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to Simon Montagu from comment #9)
> I'm not following: the line with ::-moz-placeholder was only added just now,
> in bug 737786. I believe that adding it wasn't necessary and it can just go
> (though I haven't tested that doing that fixes the regression in alignment)

It was using .placeholder before. So, the line wasn't really *just* added. Actually, the new line doesn't work so it's being ignored.

Comment 11

[Simon Montagu :smontagu]

(In reply to Mounir Lamouri (:mounir) from comment #10)
> It was using .placeholder before.

Not in this particular rule, AFAICT from http://hg.mozilla.org/mozilla-central/diff/6b9ba4944378/layout/style/forms.css#l1.30

Comment 12

[Mounir Lamouri (:mounir)]

Ok, I think I have a better idea of what was happening. The issue isn't about the invalid forms.css rules but the ::-moz-placeholder rules restriction. IOW, this commit is at fault: https://hg.mozilla.org/mozilla-central/rev/0f28fd24bdf3

I will fix this bug and open a new one for the LTR/RTL issue which, I belive, might be a Firefox issue because I do not understand why the searchbox has a working placeholder but not the URL bar.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 14

[Dão Gottwald [:dao]]

(In reply to Mounir Lamouri (:mounir) from comment #12)
> I will fix this bug and open a new one for the LTR/RTL issue which, I
> belive, might be a Firefox issue because I do not understand why the
> searchbox has a working placeholder but not the URL bar.

Because the search box doesn't use uri-element-right-align.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 16

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9195d74558f7

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9195d74558f7

Comment 18

[Mounir Lamouri (:mounir)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 737786
User impact if declined: forms.css has an invalid rule, which makes it not applied when needed. The additions was actually not needed so removing it simply fix the issue and is harmless
Risk to taking this patch (and alternatives if risky): nearly no risk, it is just making the CSS rules working as they were.
String or UUID changes made by this patch: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8eb5cd7c5c42