Closed Bug 1333256 Opened 8 years ago Closed 8 years ago

Bring back the insecure field warning Learn More text

Categories

(Toolkit :: Password Manager, defect, P2)

Product:
Toolkit
Component:
Password Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Iteration:
54.1 - Feb 6
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 + fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [FxPrivacy])

Attachments

(5 files)

Screen shot for Fx52 (ignore the footer)
33.69 KB, image/png
Details
Screen shot for Fx53 (ignore the footer)
38.67 KB, image/png
Details
Bug 1333256 - Bring back the insecure field warning Learn More text in bold on beta. l10n=flod a=jcristau
59 bytes, text/x-review-board-request
adw
: review+
jcristau
: approval-mozilla-beta+
Details
Bug 1333256 - Bring back the insecure field warning Learn More text in bold.
59 bytes, text/x-review-board-request
johannh
: review+
Details
Bug 1333256 - Bring back the insecure field warning Learn More text in bold on aurora. l10n=flod
59 bytes, text/x-review-board-request
MattN
: review+
ritu
: approval-mozilla-beta+
Details
UX has reversed the decision from bug 1318537 and wants the insecure login form warning Learn More text back. This time it needs to be a heavier font-weight than the regular text but not "bold". It also needs to be in the same flow as the warning text so that it's not always on its own line.
Comment on attachment 8829742 [details] Bug 1333256 - Bring back the insecure field warning Learn More text in bold on beta. l10n=flod a=jcristau https://reviewboard.mozilla.org/r/106744/#review107738 Hi Flod, sorry to do this but UX would like to re-add Learn More on 53 and have it wrap after the warning text. I think this is the safest/simplest approach to implement that. Luckily we already had the Learn More string leftover there from when we removed it during Aurora. Let me know what you think. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1335 (Diff revision 1) > - return this._stringBundle.GetStringFromName("insecureFieldWarningDescription"); > + return this._stringBundle.GetStringFromName("insecureFieldWarningDescription") + " " + > + this._stringBundle.GetStringFromName("insecureFieldWarningLearnMore"); Is this string concatenation with a space okay? See my localization note. ::: toolkit/content/widgets/autocomplete.xml:1541 (Diff revision 1) > + <method name="_getSearchTokens"> > + <parameter name="aSearch"/> > + <body> > + <![CDATA[ > + return [this._learnMoreString.toLowerCase()]; > + ]]> > + </body> > + </method> Returning the Learn More string here makes it the text bolder like the search string in the awesomebar. I tested that this works with emoji and it's using shared logic from the awesomebar so it should work fine. The toLowerCase doesn't actually affect the text that is visible, just the logic for bolding. ::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:72 (Diff revision 1) > +# In English a non-breaking space is used between "Learn" and "More" to keep them together upon wrapping. > +# Only do this if the translation is less than 12 characters. > +insecureFieldWarningLearnMore = Learn More Is it okay to change the space to a non-breaking space in English? Is it okay to add these localization notes?
Comment on attachment 8829742 [details] Bug 1333256 - Bring back the insecure field warning Learn More text in bold on beta. l10n=flod a=jcristau https://reviewboard.mozilla.org/r/106744/#review107770 Definitely not OK to play with strings like this. Concatenating and hard-coding spaces is one, expecting localizers to calculate the number of characters and figuring out if they should use or not use a breaking space is another. I'm also lost on the screenshots. The one for 53 is the wanted outcome of this bug? Is there a way to fix this in 53 without touching strings, i.e. adding the existing "Learn More" after the description and styling it as best as possible? For 54 I'd suggest to land a proper fix, with a new string where the "Learn more" link is a placeholder. Assuming you can style that placeholder as you want (e.g. can't force a no-wrap to all languages). insecureFieldWarningDescription2 = This connection is not secure. Logins entered here could be compromised. %S
Attachment #8829742 - Flags: review?(francesco.lodolo) → review-
Comment on attachment 8829742 [details] Bug 1333256 - Bring back the insecure field warning Learn More text in bold on beta. l10n=flod a=jcristau https://reviewboard.mozilla.org/r/106744/#review107812 ::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:72 (Diff revision 1) > +# In English a non-breaking space is used between "Learn" and "More" to keep them together upon wrapping. > +# Only do this if the translation is less than 12 characters. > +insecureFieldWarningLearnMore = Learn More Besides the localization issue already explained, is this comment even correct? I assume the "12 characters" limit is determined based on the preceding description, which will have a different length when localized. I also wonder if this non breaking space would cause the entire link to be displayed on a line of its own in some cases (different OS, font type and size).
Overall I think Ryan from UX would care less about the non-breaking space (though I'm not sure why we wouldn't be able to use it on English-only) than the Learn More link being on the last line of the warning text if it fits. (In reply to Francesco Lodolo [:flod] from comment #4) > Comment on attachment 8829742 [details] > Bug 1333256 - Bring back the insecure field warning Learn More text on beta. > > Definitely not OK to play with strings like this. Concatenating and > hard-coding spaces is one, Did you mean to say "problem" or something like that? I'm not sure if you're saying it's a problem or not since a word is missing. > … expecting localizers to calculate the number of > characters and figuring out if they should use or not use a breaking space > is another. It was an optional request that seemed simple. Are you fine with us just adding the non-breaking space for English on Beta? > I'm also lost on the screenshots. The one for 53 is the wanted outcome of > this bug? The outcome of the bug is to bring back the Learn More string on 52-54. The patch I attached is for 52 which is why it says "beta" in the name. There are different strings on 52 and 53 and I'm only focusing on a solution for 52 for now as 53 is more straightforward. > Is there a way to fix this in 53 without touching strings, i.e. adding the > existing "Learn More" after the description and styling it as best as > possible? That's what I'm doing with a space character separator which I think you're not okay with? If you're not okay with a space character which gets temporarily used internally until the XBL code wraps the learn more text in an DOM element, I don't understand how that is better than me putting the code in a separate DOM element manually e.g. > <description>This connection is not secure. Logins entered here could be compromised. <span>Learn More</span></description> The net result is the same. > For 54 I'd suggest to land a proper fix, with a new string where the "Learn > more" link is a placeholder. Assuming you can style that placeholder as you > want (e.g. can't force a no-wrap to all languages). > > insecureFieldWarningDescription2 = This connection is not secure. Logins > entered here could be compromised. %S That's already the plan for 53+ as I already landed a string exactly like that. I'm focusing on 52 first as that's where I don't have the "%S". (In reply to Francesco Lodolo [:flod] from comment #5) > Comment on attachment 8829742 [details] > Bug 1333256 - Bring back the insecure field warning Learn More text on beta. > > https://reviewboard.mozilla.org/r/106744/#review107812 > > ::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:72 > (Diff revision 1) > > +# In English a non-breaking space is used between "Learn" and "More" to keep them together upon wrapping. > > +# Only do this if the translation is less than 12 characters. > > +insecureFieldWarningLearnMore = Learn More > > Besides the localization issue already explained, is this comment even > correct? I assume the "12 characters" limit is determined based on the > preceding description, which will have a different length when localized. I just gave it as a rough estimate of the minimum username/password field size used on sites. In reality no single word in this text should be too wide or I think it will be clipped on a narrow login field. > I also wonder if this non breaking space would cause the entire link to be > displayed on a line of its own in some cases (different OS, font type and > size). Yes, but that's the desired look when there isn't room for "Learn" and "More" to be together at the end of the insecureFieldWarningDescription.
Flags: needinfo?(francesco.lodolo)
Sorry, this is getting confusing, and I didn't contribute to make it clearer. I started from comment 3, where the NI was, and that talks about 53, which is aurora ("beta" is mentioned only in the patch name, and I missed it). From there I assumed that the 52 screenshot was the current situation, and we needed to restore the link in 53 (and maybe 54). If I (hopefully) understand it correctly, we already have the correct strings (with placeholder) on aurora and central, we only need to fix Beta (52). https://hg.mozilla.org/releases/mozilla-aurora/file/default/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties#l75 > Overall I think Ryan from UX would care less about the non-breaking space > (though I'm not sure why we wouldn't be able to use it on English-only) than > the Learn More link being on the last line of the warning text if it fits. You absolutely can use it in English, sorry if that wasn't clear. There's also no need to point it out in the localization comment. > (In reply to Francesco Lodolo [:flod] from comment #4) > > Comment on attachment 8829742 [details] > > Bug 1333256 - Bring back the insecure field warning Learn More text on beta. > > > > Definitely not OK to play with strings like this. Concatenating and > > hard-coding spaces is one, > > Did you mean to say "problem" or something like that? I'm not sure if you're > saying it's a problem or not since a word is missing. Argh. Yes, I meant "is one problem". > It was an optional request that seemed simple. Are you fine with us just > adding the non-breaking space for English on Beta? > > The outcome of the bug is to bring back the Learn More string on 52-54. The > patch I attached is for 52 which is why it says "beta" in the name. There > are different strings on 52 and 53 and I'm only focusing on a solution for > 52 for now as 53 is more straightforward. Unfortunately touching l10n files will trigger new compare-locales runs and make existing Beta sign-offs meaningless. I'd really like to avoid that if possible. Is there any chance to land a patch without the non-breaking space change? In case I can deal with the outcomes (a matter of time to fix sign-offs for all locales). > That's what I'm doing with a space character separator which I think you're > not okay with? If you're not okay with a space character which gets > temporarily used internally until the XBL code wraps the learn more text in > an DOM element, I don't understand how that is better than me putting the > code in a separate DOM element manually e.g. > > > <description>This connection is not secure. Logins entered here could be compromised. <span>Learn More</span></description> > > The net result is the same. That's a good point, I'm just against having unnatural hard-coded concatenations living in the code. Since this is only for beta, it's OK to have that kind of code around for a couple of cycles. > I just gave it as a rough estimate of the minimum username/password field > size used on sites. In reality no single word in this text should be too > wide or I think it will be clipped on a narrow login field. That doesn't sound good :-\ https://transvision.mozfr.org/string/?entity=toolkit/chrome/passwordmgr/passwordmgr.properties:insecureFieldWarningDescription&repo=beta To recap, at this point I'm OK with landing a patch without the change to "Learn more" (if possible), or at least without the localization note.
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] from comment #7) > Sorry, this is getting confusing, and I didn't contribute to make it clearer. > > I started from comment 3, where the NI was, and that talks about 53, which > is aurora ("beta" is mentioned only in the patch name, and I missed it). > From there I assumed that the 52 screenshot was the current situation, and > we needed to restore the link in 53 (and maybe 54). OK, sorry, that was my fault for saying 53 there. > If I (hopefully) understand it correctly, we already have the correct > strings (with placeholder) on aurora and central, we only need to fix Beta > (52). > https://hg.mozilla.org/releases/mozilla-aurora/file/default/toolkit/locales/ > en-US/chrome/passwordmgr/passwordmgr.properties#l75 Yes, except the aurora and m-c strings don't have the non-breaking space either because I forgot about it. > > Overall I think Ryan from UX would care less about the non-breaking space > > (though I'm not sure why we wouldn't be able to use it on English-only) than > > the Learn More link being on the last line of the warning text if it fits. > > You absolutely can use it in English, sorry if that wasn't clear. There's > also no need to point it out in the localization comment. OK > > (In reply to Francesco Lodolo [:flod] from comment #4) > > > Comment on attachment 8829742 [details] > > > Bug 1333256 - Bring back the insecure field warning Learn More text on beta. > > > > > > Definitely not OK to play with strings like this. Concatenating and > > > hard-coding spaces is one, > > > > Did you mean to say "problem" or something like that? I'm not sure if you're > > saying it's a problem or not since a word is missing. > > Argh. Yes, I meant "is one problem". > > > > It was an optional request that seemed simple. Are you fine with us just > > adding the non-breaking space for English on Beta? > > > > The outcome of the bug is to bring back the Learn More string on 52-54. The > > patch I attached is for 52 which is why it says "beta" in the name. There > > are different strings on 52 and 53 and I'm only focusing on a solution for > > 52 for now as 53 is more straightforward. > > Unfortunately touching l10n files will trigger new compare-locales runs and > make existing Beta sign-offs meaningless. I'd really like to avoid that if > possible. Is there any chance to land a patch without the non-breaking space > change? OK, I tried to add the non-breaking space in the code but it seems like the emphasis logic (from the awesomebar) doesn't like the non-breaking space. Ryan, are you fine with no non-breaking space in 52? I could try to do it from CSS but it would be more hacks as I would have to get the current locale code to only do it for en-US builds. > In case I can deal with the outcomes (a matter of time to fix sign-offs for > all locales). > > > That's what I'm doing with a space character separator which I think you're > > not okay with? If you're not okay with a space character which gets > > temporarily used internally until the XBL code wraps the learn more text in > > an DOM element, I don't understand how that is better than me putting the > > code in a separate DOM element manually e.g. > > > > > <description>This connection is not secure. Logins entered here could be compromised. <span>Learn More</span></description> > > > > The net result is the same. > > That's a good point, I'm just against having unnatural hard-coded > concatenations living in the code. Since this is only for beta, it's OK to > have that kind of code around for a couple of cycles. OK, thanks. I've attached a minimal patch that doesn't touch string files and doesn't deal with the non-breaking space. I can do a separate patch for the space depending on what Ryan says. > > I just gave it as a rough estimate of the minimum username/password field > > size used on sites. In reality no single word in this text should be too > > wide or I think it will be clipped on a narrow login field. > > That doesn't sound good :-\ > https://transvision.mozfr.org/string/?entity=toolkit/chrome/passwordmgr/ > passwordmgr.properties:insecureFieldWarningDescription&repo=beta Yeah, it's kinda a problem for English too. Bug 1330731 is one proposal but I haven't look into whether that is straightforward. > To recap, at this point I'm OK with landing a patch without the change to > "Learn more" (if possible), or at least without the localization note. Great, thanks.
Flags: needinfo?(rfeeley)
Priority: -- → P2
Comment on attachment 8829742 [details] Bug 1333256 - Bring back the insecure field warning Learn More text in bold on beta. l10n=flod a=jcristau https://reviewboard.mozilla.org/r/106744/#review108488 As said, carry forward the r+ if we really need to change the string too.
Attachment #8829742 - Flags: review?(francesco.lodolo) → review+
It needs the non-breaking space in Learn More. More users will end up seeing this than many other parts of the Ui (at least that I've worked on). Though it's not a link, it's intended to signify that the row is a link. Breaking it up means that more users will open SUMO inadvertently.
Flags: needinfo?(rfeeley)
Comment on attachment 8829742 [details] Bug 1333256 - Bring back the insecure field warning Learn More text in bold on beta. l10n=flod a=jcristau Approval Request Comment [Feature/Bug causing the regression]: Insecure field warning [User impact if declined]: No "Learn More" text to indicate that clicking will open a SUMO article [Is this code covered by automated tests?]: Yes, the feature is. [Has the fix been verified in Nightly?]: No, this patch needs to land only on Beta since Nightly and Aurora need a different approach [Needs manual test from QE? If yes, steps to reproduce]: I can do a try push for QE to verify if you'd like. [List of other uplifts needed for the feature/fix]: This doesn't depend on others not already uplifted. [Is the change risky?]: No [Why is the change risky/not risky?]: It uses existing emphasizing logic from the awesomebar for the Learn More text. [String changes made/needed]: Yes, added a non-breaking space between "Learn" and "More" for English only. flod approved this.
Attachment #8829742 - Flags: approval-mozilla-beta?
Attachment #8829742 - Flags: review?(selee) → review?(adw)
Comment on attachment 8829742 [details] Bug 1333256 - Bring back the insecure field warning Learn More text in bold on beta. l10n=flod a=jcristau https://reviewboard.mozilla.org/r/106744/#review109314
Attachment #8829742 - Flags: review?(adw) → review+
Iteration: --- → 54.1 - Feb 6
Flags: qe-verify?
Whiteboard: [FxPrivacy]
In case your uplift searches miss this bug: the patch in question needs to land directly on Beta since it needs a different approach altogether than Nightly + Aurora.
Flags: needinfo?(jcristau)
Keywords: leave-open
Comment on attachment 8829742 [details] Bug 1333256 - Bring back the insecure field warning Learn More text in bold on beta. l10n=flod a=jcristau add "Learn More" text to insecure field warning, for beta52, l10n-acked by flod Thanks Matt, this did show up in my uplift pass yesterday but didn't have Drew's r+ yet :)
Flags: needinfo?(jcristau)
Attachment #8829742 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Backed out Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=147b55311d5350e6d7022d031f725934d6c717a0 Failure log mochitest: https://treeherder.mozilla.org/logviewer.html#?job_id=73330338&repo=mozilla-beta > TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html | Check all menuitems are displayed correctly. Checking array entry #0 - got "This connection is not secure. Logins entered here could be compromised. Learn More", expected "This connection is not secure. Logins entered here could be compromised." Failure log xpcshell: https://treeherder.mozilla.org/logviewer.html#?job_id=73306959&repo=mozilla-beta > TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js | test_all_patterns - [test_all_patterns : 473] "This connection is not secure. Logins entered here could be compromised. Learn More" == "This connection is not secure. Logins entered here could be compromised."
Flags: needinfo?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
Was there ever a decision on the two string variants yet? https://dxr.mozilla.org/mozilla-central/search?q=insecureFieldWarningDescription+ext%3Aproperties&redirect=false
Flags: needinfo?(rfeeley)
The tests will need to be updated based on the decision on comment 22 but otherwise I think this is all the areas that need to be touched.
Feel free to autoland if it gets r+.
Comment on attachment 8836788 [details] Bug 1333256 - Bring back the insecure field warning Learn More text in bold. https://reviewboard.mozilla.org/r/112112/#review113460 Looks good. I'm not landing yet because of the "change" below. ::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:78 (Diff revision 1) > -insecureFieldWarningDescription = This connection is not secure. Logins entered here could be compromised. > # LOCALIZATION NOTE (insecureFieldWarningDescription2, insecureFieldWarningDescription3): > # %1$S will contain insecureFieldWarningLearnMore and look like a link to indicate that clicking will open a tab with support information. > insecureFieldWarningDescription2 = This connection is not secure. Logins entered here could be compromised. %1$S > insecureFieldWarningDescription3 = Logins entered here could be compromised. %1$S > -insecureFieldWarningLearnMore = Learn More > +insecureFieldWarningLearnMore = Learn More What changed here? It also appears in the diff so it's not a problem with reviewboard.
Attachment #8836788 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #28) > What changed here? It also appears in the diff so it's not a problem with > reviewboard. It's a non-breaking space as discussed many other times on this bug.
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/0867bd82c562 Bring back the insecure field warning Learn More text in bold. r=johannh
Flags: qe-verify?
Flags: qe-verify-
Flags: needinfo?(rfeeley)
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 8843130 [details] Bug 1333256 - Bring back the insecure field warning Learn More text in bold on aurora. l10n=flod Approval Request Comment [Feature/Bug causing the regression]: Insecure field warning [User impact if declined]: No "Learn More" text to indicate that clicking will open a SUMO article [Is this code covered by automated tests?]: Yes, the feature is. [Has the fix been verified in Nightly?]: Yes, this Aurora patch uses the same approach as the m-c one that landed. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: This doesn't depend on others not already uplifted. [Is the change risky?]: No [Why is the change risky/not risky?]: It uses existing emphasizing logic from the awesomebar for the Learn More text. This has been on Beta and Nightly for a few weeks. [String changes made/needed]: Yes, added a non-breaking space between "Learn" and "More" for English only. flod approved this.
Attachment #8843130 - Flags: review?(jhofmann) → review+
Attachment #8843130 - Flags: approval-mozilla-aurora?
> In case I can deal with the outcomes (a matter of time to fix sign-offs for all locales). This (and the r+) assumed the change would land at the time of the comment, not the week-end before Merge Day. At this point, can we get this landed directly in mozilla-beta as soon as the cycle starts, and skip aurora? The risk of loosing the existing translation is too high (one tool will remove them because English changed, and I won't have time to fix it).
Flags: needinfo?(MattN+bmo)
(In reply to Francesco Lodolo [:flod] from comment #34) > > In case I can deal with the outcomes (a matter of time to fix sign-offs for all locales). > > This (and the r+) assumed the change would land at the time of the comment, > not the week-end before Merge Day. The 52 beta part landed a long time ago… > At this point, can we get this landed directly in mozilla-beta as soon as > the cycle starts, and skip aurora? The risk of loosing the existing > translation is too high (one tool will remove them because English changed, > and I won't have time to fix it). Just to clarify, this fix already landed in 52 and 54 so only needs to land in 53 which will move from Aurora to Beta next week. Are you saying that it's easier for you to land in 53 when it goes to Beta instead of now on Aurora?
Flags: needinfo?(MattN+bmo) → needinfo?(francesco.lodolo)
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #35) > Just to clarify, this fix already landed in 52 and 54 so only needs to land > in 53 which will move from Aurora to Beta next week. Are you saying that > it's easier for you to land in 53 when it goes to Beta instead of now on > Aurora? Exactly. The tool that would invalidate the changed string only works against aurora. If this lands when 53 is on Beta, I will still need to go through the sign-offs, but we won't risk losing translations.
Flags: needinfo?(francesco.lodolo)
That sounds fine. I'll let this sit for now, and after the merge on Monday I'll change the flag to one for a beta uplift request.
Attachment #8843130 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8843130 [details] Bug 1333256 - Bring back the insecure field warning Learn More text in bold on aurora. l10n=flod This fix shipped in 52 and is on 54, Beta53+
Attachment #8843130 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: