Encourage user to set email in issue reporter

Encourage user to set email in issue reporter

[Wladimir Palant, 2012-11-22 14:16:29]

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport.js
File chrome/content/ui/sendReport.js (right):

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport...
chrome/content/ui/sendReport.js:1326: document.documentElement.canAdvance =
true;
I am mostly sure that canAdvance = true is set automatically after switching
pages. Is this necessary?

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport...
chrome/content/ui/sendReport.js:1337: updateEmail();
This will already set the canAdvance property, I don't see much point doing it
in this function as well.

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport...
chrome/content/ui/sendReport.js:1390: E("email").setAttribute("value", value);
// https://bugzilla.mozilla.org/show_bug.cgi?id=111486
Please add a proper comment - the bug link is additional info, not an
explanation.

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport...
chrome/content/ui/sendReport.js:1405: E("emailLabel").disabled = anonymous;
I don't think we ever disable labels.

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport...
chrome/content/ui/sendReport.js:1409: }
The interaction of these two functions is pretty complex, depending on the order
in which they are called. For example: use enters an email address, checks
"anonymous" checkbox then. Email address will still be sent with your code.
Please make updateEmail() consider the anonymous checkbox instead, to make the
logic obvious - same function should be called regardless of whether the
textfield or the checkbox was updated.

http://codereview.adblockplus.org/8937025/diff/1/chrome/locale/en-US/sendRepo...
File chrome/locale/en-US/sendReport.dtd (right):

http://codereview.adblockplus.org/8937025/diff/1/chrome/locale/en-US/sendRepo...
chrome/locale/en-US/sendReport.dtd:168: <!ENTITY emailComment.label          
"STAB: Text about why e-mail is important. May be we also should rename Enter
comment page title to something else?">
Please don't get all violent here - s/STAB/STUB/

Also, this obviously cannot land without proper text... How about: "We encourage
you to enter a valid email address so that we can contact you if there are
questions about your report. It will also allow us to recognize your
contributions and to prioritize them higher."

http://codereview.adblockplus.org/8937025/diff/1/chrome/locale/en-US/sendRepo...
chrome/locale/en-US/sendReport.dtd:172: <!ENTITY email.label                 
"Email for further inquiries:">
This can simply be "Email" if we have proper text above.

[Andrey Novikov, 2012-11-22 14:44:15]

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport.js
File chrome/content/ui/sendReport.js (right):

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport...
chrome/content/ui/sendReport.js:1326: document.documentElement.canAdvance =
true;
It stays disabled in my case when I press Back on Comments page.

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport...
chrome/content/ui/sendReport.js:1337: updateEmail();
On 2012/11/22 14:16:29, Wladimir Palant wrote:
> This will already set the canAdvance property, I don't see much point doing it
> in this function as well.

Done.

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport...
chrome/content/ui/sendReport.js:1390: E("email").setAttribute("value", value);
// https://bugzilla.mozilla.org/show_bug.cgi?id=111486
On 2012/11/22 14:16:29, Wladimir Palant wrote:
> Please add a proper comment - the bug link is additional info, not an
> explanation.

Done.

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport...
chrome/content/ui/sendReport.js:1405: E("emailLabel").disabled = anonymous;
It looks much more user friendly in UI, have a try. But if you wish I will
remove it.

http://codereview.adblockplus.org/8937025/diff/1/chrome/content/ui/sendReport...
chrome/content/ui/sendReport.js:1409: }
It sends empty value in that case. Redesigned it.

http://codereview.adblockplus.org/8937025/diff/1/chrome/locale/en-US/sendRepo...
File chrome/locale/en-US/sendReport.dtd (right):

http://codereview.adblockplus.org/8937025/diff/1/chrome/locale/en-US/sendRepo...
chrome/locale/en-US/sendReport.dtd:168: <!ENTITY emailComment.label          
"STAB: Text about why e-mail is important. May be we also should rename Enter
comment page title to something else?">
On 2012/11/22 14:16:29, Wladimir Palant wrote:
> Please don't get all violent here - s/STAB/STUB/
> 
> Also, this obviously cannot land without proper text... How about: "We
encourage
> you to enter a valid email address so that we can contact you if there are
> questions about your report. It will also allow us to recognize your
> contributions and to prioritize them higher."

Done.

http://codereview.adblockplus.org/8937025/diff/1/chrome/locale/en-US/sendRepo...
chrome/locale/en-US/sendReport.dtd:172: <!ENTITY email.label                 
"Email for further inquiries:">
On 2012/11/22 14:16:29, Wladimir Palant wrote:
> This can simply be "Email" if we have proper text above.

Done.

[Wladimir Palant, 2012-11-22 15:08:21]

http://codereview.adblockplus.org/8937025/diff/3002/chrome/content/ui/sendRep...
File chrome/content/ui/sendReport.js (right):

http://codereview.adblockplus.org/8937025/diff/3002/chrome/content/ui/sendRep...
chrome/content/ui/sendReport.js:1390: E("email").value = "";
Please don't do that, disabling it is sufficient - people might actually change
their mind.

http://codereview.adblockplus.org/8937025/diff/3002/chrome/content/ui/sendRep...
chrome/content/ui/sendReport.js:1393: E("email").setAttribute("value", value);
// required for persist to work on textbox, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=111486
Please put that comment on the line(s) before - we should at least *try* to
limit the line length to 80 characters. Plus, it makes the comment more visible.

[Andrey Novikov, 2012-11-23 06:32:16]

http://codereview.adblockplus.org/8937025/diff/3002/chrome/content/ui/sendRep...
File chrome/content/ui/sendReport.js (right):

http://codereview.adblockplus.org/8937025/diff/3002/chrome/content/ui/sendRep...
chrome/content/ui/sendReport.js:1390: E("email").value = "";
It will be not obvious that nothing will be sent if the text remains in the
field.

http://codereview.adblockplus.org/8937025/diff/3002/chrome/content/ui/sendRep...
chrome/content/ui/sendReport.js:1393: E("email").setAttribute("value", value);
// required for persist to work on textbox, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=111486
On 2012/11/22 15:08:21, Wladimir Palant wrote:
> Please put that comment on the line(s) before - we should at least *try* to
> limit the line length to 80 characters. Plus, it makes the comment more
visible.

Done.

[Wladimir Palant, 2012-11-23 06:44:38]

LGTM

Please don't push that change yes, Adblock Plus 2.2.1 needs to be released
first.