Closed Bug 810796 Opened 9 years ago Closed 9 years ago

Fix invalid CSS in forms.css

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox18 --- unaffected
firefox19 + fixed
firefox20 --- fixed

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.  ;)
Duplicate of this bug: 811821
While we understand the CSS is invalid, it's not clear what the user impact is here if left unfixed.
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).
(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?
Keywords: rtl
Yeah I think the text should be left-aligned if the textbox is LTR.
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)
(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.
(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
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.
Attachment #682434 - Attachment is obsolete: true
Attachment #682435 - Attachment is obsolete: true
Keywords: rtl
Attached patch PatchSplinter Review
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #683570 - Flags: review?(bzbarsky)
(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 on attachment 683570 [details] [diff] [review]
Patch

r=me
Attachment #683570 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9195d74558f7
Flags: in-testsuite-
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/9195d74558f7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
Attachment #683570 - Flags: approval-mozilla-aurora?
Attachment #683570 - Flags: approval-mozilla-beta+
Attachment #683570 - Flags: approval-mozilla-beta+
Attachment #683570 - Flags: approval-mozilla-aurora?
Attachment #683570 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.