Last Comment Bug 739208 - CSS warning when the account provisioner is displayed
: CSS warning when the account provisioner is displayed
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 709945
  Show dependency treegraph
 
Reported: 2012-03-26 06:36 PDT by Florian Quèze [:florian] [:flo]
Modified: 2012-04-03 16:38 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.96 KB, patch)
2012-03-26 11:22 PDT, :aceman
bwinton: ui‑review-
Details | Diff | Splinter Review
Comparison of having -moz-placeholder, or no rule at all... (43.03 KB, image/png)
2012-03-27 11:04 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details
patch v2 (3.06 KB, patch)
2012-04-02 11:02 PDT, :aceman
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] 2012-03-26 06:36:56 PDT
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] [:flo] 2012-03-26 06:42:28 PDT
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] [:flo] 2012-03-26 06:47:42 PDT
Possibly caused by bug 700536 and related to bug 709945.
Comment 3 :aceman 2012-03-26 06:59:41 PDT
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 2012-03-26 11:22:22 PDT
Created attachment 609392 [details] [diff] [review]
patch

This makes the change and the warning does not happen. But I can't see any visual change.
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-03-27 10:43:21 PDT
Comment on attachment 609392 [details] [diff] [review]
patch

Stealing this review request.
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-03-27 10:45:53 PDT
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 2012-03-27 11:03:47 PDT
Thanks to florian:)
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-03-27 11:04:13 PDT
Created attachment 609790 [details]
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) - (Needinfo me!) 2012-03-27 11:05:17 PDT
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) - (Needinfo me!) 2012-03-27 11:05:45 PDT
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 2012-03-27 11:20:59 PDT
That is what I said in comment 4 (no visual change) :)
Comment 12 Blake Winton (:bwinton) (:☕️) 2012-04-02 10:48:55 PDT
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 2012-04-02 11:02:41 PDT
Created attachment 611520 [details] [diff] [review]
patch v2
Comment 14 Blake Winton (:bwinton) (:☕️) 2012-04-02 11:47:00 PDT
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] 2012-04-03 16:38:02 PDT
http://hg.mozilla.org/comm-central/rev/3c7492c88129

Note You need to log in before you can comment on or make changes to this bug.