551747 - Use HTML 5 'placeholder' attribute instead of 'emptyText'

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Stefan [:stefanh]]

On 1.9.3, there's now a placeholder attribute that can be used instead of the emptytext attribute.

Comment 1

[Nomis101]

Attachment: WIP

In the style of attachment.cgi?id=429971, this WIP patch changes all emptytext= to placeholder=.
http://mxr.mozilla.org/comm-central/search?string=emptytext=

Comment 2

[Nomis101]

Attachment: fix

I will finish my patch and add an reviewer after my patch from bug 566282 has landed.

Comment 3

[Nomis101]

Attachment: Fix [already checked-in]

This patch will change all "emptytext" to "placeholder.
https://developer.mozilla.org/en/XUL/Attribute/emptytext
I've also changed the strings name from *.emptytext to *.placeholder, like it was done in Bug 550186 for SM.
This patch will also fix the missing placeholder text in the quicksearch bar.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Thanks for this patch too! :)

I don't think we want to be changing the localizable string names just for consistency.  As was raised in the seamonkey bug, there is no semantic change, so we'd just be making busywork for the localizers.  Also, I can't review the suite/ bit, so if we don't make that change, I can review the patch :)

Looping in standard8 and sipaq for their thoughts on changing the entity names from "blah.emptytext" to "blah.placeholder" for consistency.

Comment 5

[Simon Paquet [:sipaq]]

I'm generally opposed to large entity name changes just for consistency, but since this change is isolated, small and lets us keep parity with Suite code, I think it is acceptable from an l10n point of view.

However, once this is checked in, it would certainly help to inform localizers in mozilla.dev.l10n about this. It seems that this wasn't done for the Suite bug, but we could do better here.

Comment 6

[Stefan [:stefanh]]

(In reply to comment #4)
> Also, I can't review the
> suite/ bit, so if we don't make that change, I can review the patch :)

It would then be appreciated if you filed a bug for the suite part (it wasn't possible to fix mailnews/TB when suite was done - TB was on 1.9.2 then).

Comment 7

[Nomis101]

(In reply to comment #4)
> I don't think we want to be changing the localizable string names just for
> consistency.
I was also unsure if this is really needed. But this is only a small change, I changed all those strings for my localized build in 2 minutes. Anyhow, It's easy to make a new patch without string changes.


(In reply to comment #4)
> Also, I can't review the
> suite/ bit, so if we don't make that change, I can review the patch :)
I can split this patch into one mail/mailnews and one suite part. Than you can review the first patch and a SM reviewer can review the second patch. Is this OK?


(In reply to comment #5)
> However, once this is checked in, it would certainly help to inform localizers
> in mozilla.dev.l10n about this. It seems that this wasn't done for the Suite
> bug, but we could do better here.
Yes, good idea, I will do this after this Bug is fixed.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 468688 [details] [diff] [review]
Fix [already checked-in]

If sipaq is cool with it, I'm cool with it.  Please do make a post to the l10n group once this lands as he requests.

Please seek seamonkey signoff/review for the change too.  I don't think you need to split the patch/bug or what not.

Comment 9

[Nomis101]

@Neil: Would be nice if you can review the suite part of my patch. This is only one file, am-copies.dtd. I've selected you, because you already reviewed the similar Bug 550186.

Comment 10

[[e-mail]]

Comment on attachment 468688 [details] [diff] [review]
Fix [already checked-in]

>diff --git a/mail/components/addrbook/content/abCardOverlay.xul b/mail/components/addrbook/content/abCardOverlay.xul
>diff --git a/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd b/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
Hey, I don't suppose I could prevail on you to port these changes to suite's version of abCardOverlay.xul and abCardOverlay.dtd by any chance? ;-)

Comment 11

[Nomis101]

(In reply to comment #10)
> Comment on attachment 468688 [details] [diff] [review]
> Fix
> 
> >diff --git a/mail/components/addrbook/content/abCardOverlay.xul b/mail/components/addrbook/content/abCardOverlay.xul
> >diff --git a/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd b/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
> Hey, I don't suppose I could prevail on you to port these changes to suite's
> version of abCardOverlay.xul and abCardOverlay.dtd by any chance? ;-)

No problem, I can do this after this bug is fixed. :-)

Comment 12

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/bfdbb8ccf807

Comment 13

[Nomis101]

Attachment: [checked in] Follow-up (abCardOverlay.js)

Neil discovered, that I've overlooked abCardOverlay.js (in Bug 590932). This patch will also alter this last missing file for mail.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Could you also remove these lines:

    3.18 +        if (this.hasAttribute("placeholder"))
    3.19 +          this.placeholder = this.getAttribute("placeholder");

element.placeholder = "foo" is equivalent to element.setAttribute("placeholder", "foo"), so this code doesn't change anything.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #14)
> Could you also remove these lines:
> 
>     3.18 +        if (this.hasAttribute("placeholder"))
>     3.19 +          this.placeholder = this.getAttribute("placeholder");
> 
> element.placeholder = "foo" is equivalent to
> element.setAttribute("placeholder", "foo"), so this code doesn't change
> anything.

Assuming the XUL binding didn't change extensively, this is intentional and (probably) still required because of the sequence of events during the initialization phase.  (If you remove it and the empty text is still there at startup, maybe the XUL binding changed a bit.)

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 469892 [details] [diff] [review]
[checked in] Follow-up (abCardOverlay.js)

Checked in: http://hg.mozilla.org/comm-central/rev/ee041a8548ca