739208 - CSS warning when the account provisioner is displayed

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Florian Quèze [:florian]]

Warning: Unknown pseudo-class or pseudo-element 'placeholder'.  Ruleset ignored due to bad selector.
Source File: chrome://messenger/skin/newmailaccount/accountProvisioner.css
Line: 224

Line 224 is:
#search input[type="text"]:placeholder {

The same code exists in all 3 themes:
/mail/themes/qute/mail/newmailaccount/accountProvisioner.css
/mail/themes/pinstripe/mail/newmailaccount/accountProvisioner.css
/mail/themes/gnomestripe/mail/newmailaccount/accountProvisioner.css

I think ":placeholder" just needs to be replaced with ":-moz-placeholder", see https://developer.mozilla.org/en/CSS/:-moz-placeholder

Comment 1

[Florian Quèze [:florian]]

Aceman, not sure if you are looking for more easy cleanup bugs, but if my analysis in comment 0 is right and it isn't filed anywhere else yet with a patch waiting for review, this seems easy; so CC'ing you just in case :-).

Comment 2

[Florian Quèze [:florian]]

Possibly caused by bug 700536 and related to bug 709945.

Comment 3

[:aceman]

Thanks,
it is in 4 files:
http://mxr.mozilla.org/comm-central/search?string=%3Aplaceholder&find=.css&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

No other occurences in comm-central.
I couldn't find a dupe. I'll do it.
I just usually don't care much for new experimental modules :)

Comment 4

[:aceman]

Attachment: patch

This makes the change and the warning does not happen. But I can't see any visual change.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 609392 [details] [diff] [review]
patch

Stealing this review request.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 609392 [details] [diff] [review]
patch

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

This looks right to me!  Thanks aceman!

Comment 7

[:aceman]

Thanks to florian:)

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Comparison of having -moz-placeholder, or no rule at all...

Hm.  So I'm reconsidering my r+ on the patch for this.

While the code looks good, I'm not sure the rule actually gives us anything that we want.  The difference between having -moz-placeholder vs. just tearing that rule out seem negligible.  See attached.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

I'm going to take off checkin-needed for now until we resolve whether or not we even need the rule.

bwinton: do we even need this rule?  If we were fine with the way Account Provisioner already was, we might as well excise it, since I don't think it's adding much.

-Mike

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 609392 [details] [diff] [review]
patch

Taking off my r+ until we determine whether or not we even need the rule.

Comment 11

[:aceman]

That is what I said in comment 4 (no visual change) :)

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 609392 [details] [diff] [review]
patch

Yeah, I think we can safely get rid of that rule, since it doesn't really add anything.

Thanks,
Blake.

Comment 13

[:aceman]

Attachment: patch v2

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 611520 [details] [diff] [review]
patch v2

Looks good, so I'll say ui-r=me, and r=me while I'm here.

Thanks,
Blake.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/3c7492c88129