I changed the text for the http Authentication prompt dialog - bug 1230462. The text is a bit longer now. But before this change it could have been as long as it is now in some cases. The part of the text is a realm that is sent from the server and it can have variable length. On one computer the dialog is cut (see attachment). This is Win10. I could not reproduce it on Mac, Linux and Win7 and couple of other people could not reproduce it on WinXP, Mc and Win10.

If you need info, mayhemer is the only one who can reproduce it :)

I suspect this is caused by the newlines bug 1230462 is adding, and that splitting the double \n out of the string (and using 2 separate elements) would fix this. Not sure *why* the extra newlines are causing this problem, though.

I also can't reproduce this on my Win10 machine. Honza, are you _sure_ this is broken for you? Specifically, you're not running an earlier/stale build that doesn't have the fix from bug 1230462 comment 72?

(In reply to Justin Dolske [:Dolske] from comment #3) > I also can't reproduce this on my Win10 machine. > > Honza, are you _sure_ this is broken for you? Specifically, you're not > running an earlier/stale build that doesn't have the fix from bug 1230462 > comment 72? This is a funny question, sorry :) Yes, I'm pretty sure. The screenshot was not made up in photoshop. I was also trying to find an actual fix by manipulating the css files and js code of commonDialog, starting with the latest fix [1]. No luck at all. Seems like the height of the whole window is not reflected when the main area text height (number of lines) goes over a certain threshold. According [1], the height it tries to reset to is already the height the dialog actually has, so it can't have any effect. Also I was trying with the try build, so probably nothing in my local build config. Can you add a screenshot of the dialog as it look on your screen? Maybe some major difference may appear and we find out. [1] https://hg.mozilla.org/try/diff/5aebf0c8cc2d/toolkit/components/prompts/content/commonDialog.js

[Tracking Requested - why for this release]: Ugly visual regression.

I see this on Win10 all the time when connecting to https://secure.pub.build.mozilla.org/buildapi/

Tracking 50+ for this visible regression.

Weird. I couldn't reproduce this at all before, and now I can. Enn, any suggestions here? I think comment 2 is right about the likely cause -- the only change in bug 1230462 that really affects the dialog contents is the newline. A window.sizeToContent() call was added, but apparently doesn't actually fix this.

...if I make one of the dialog buttons actually just call window.sizeToContent() when clicked, it still doesn't do anything.

Do we have a plan for addressing this regression?

If we cannot resolve this on time we could remove one of the new lines in the message. I have put 2 new lines for readability, to make warning text more visible, but we can remove it. Using 2 elements instead of 2 new lines, I think would resolve the problem too. (But I do not know code that good to fix it :)

I'm unable to reproduce this so I can't be much help, sorry.

(In reply to Dragana Damjanovic [:dragana] from comment #12) > If we cannot resolve this on time we could remove one of the new lines in > the message. I have put 2 new lines for readability, to make warning text > more visible, but we can remove it. I think this is the best option at this point. :( Can you do that patch? Otherwise a fix is likely going to need someone with XUL layout knowledge and the ability to reproduce this. I'm not sure who else knows XUL internals these days. > Using 2 elements instead of 2 new lines, I think would resolve the problem > too. (But I do not know code that good to fix it :) Unfortunately there are a few levels of APIs between the callers and the construction of the prompt, and they only support a single string being passed in for the message here. So not a simple fix.

We need to fix this in XUL, because realm can have line breaks and that will cause the same problem. I will open a new bug.

(In reply to Dragana Damjanovic [:dragana] from comment #17) > We need to fix this in XUL, because realm can have line breaks and that will > cause the same problem. I will open a new bug. bug 1295916

Honza, can you try a build from comment #16. I have removed an extra line break. Thanks!!!

(In reply to Dragana Damjanovic [:dragana] from comment #19) > Honza, can you try a build from comment #16. I have removed an extra line > break. Thanks!!! Dragana, it's still cut :(((

Unfortunately, I'd suspect the issue here is with _any_ line break, so fixing this would remove both of them from the string.

Now without both new lines.

Honza, can you try the build from comment 23.

(In reply to Dragana Damjanovic [:dragana] from comment #24) > Honza, can you try the build from comment 23. Works, let's go with it. But the informative value of the text is clearly ruined :( no one's gonna read the WARNING part.

Comment on attachment 8782148 [details] [diff] [review] bug_1277895.patch Might be worth checking to see if the patch in bug 1293242 happens to fix this. They sound similar (and Gijs commented as such), although the latter comments and fix make them sound different.

Honza, can you try this patch. I have added style="overflow: -moz-hidden-unscrollable;" as suggested in bug 1293242 comment 10

(In reply to Dragana Damjanovic [:dragana] from comment #27) > Created attachment 8785957 [details] [diff] [review] > bug_fix_dialog_size.patch > > Honza, can you try this patch. I have added style="overflow: > -moz-hidden-unscrollable;" as suggested in bug 1293242 comment 10 Still cut... :(

(In reply to Dragana Damjanovic [:dragana] from comment #27) > I have added style="overflow: > -moz-hidden-unscrollable;" as suggested in bug 1293242 comment 10 If I understand correctly, this should be applied to whatever container in the window has "overflow: hidden;" on it, not the <description> element. You can use the Inspector in the Browser Toolbox to find these elements, if any. If none are present, then the suggestion from bug 1293242 does not apply here.

Dragana: Maybe it's best to just land the patch removing the newlines landed? 50 is uplifting to beta shortly, and it would be best to get these string changes out of the way.

Honza, can you try a build from: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa3b94e0ddad This is my last try to find a work around. I do not know the prompt code so this is just a guess. Thanks.

(In reply to Dragana Damjanovic [:dragana] from comment #31) > Honza, can you try a build from: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa3b94e0ddad > > This is my last try to find a work around. I do not know the prompt code so > this is just a guess. Thanks. Sorry, still the same :(

Needs rebasing.

rebased.

has problems to apply: applying bug_1277895.patch patching file toolkit/components/passwordmgr/test/mochitest/test_prompt_http.html Hunk #4 FAILED at 179 1 out of 5 hunks FAILED -- saving rejects to file toolkit/components/passwordmgr/test/mochitest/test_prompt_http.html.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh bug_1277895.patch

(In reply to Carsten Book [:Tomcat] from comment #38) > has problems to apply: > > applying bug_1277895.patch > patching file > toolkit/components/passwordmgr/test/mochitest/test_prompt_http.html > Hunk #4 FAILED at 179 > 1 out of 5 hunks FAILED -- saving rejects to file > toolkit/components/passwordmgr/test/mochitest/test_prompt_http.html.rej > patch failed, unable to continue (try -v) > patch failed, rejects left in working directory > errors during apply, please fix and qrefresh bug_1277895.patch I have just rebased it yesterday and changed exactly this file :) Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/88570c6adbe21830848a2e3f2f88cb1557e0dd29 Bug 1277895 - Remove one new line from http auth prompt message. r=dolske

https://hg.mozilla.org/integration/mozilla-inbound/rev/4236645e6bdf2bb3cc7dc5324544be14a58fc00e Bug 1277895 - fix a test.r=dolske

https://hg.mozilla.org/mozilla-central/rev/88570c6adbe2 https://hg.mozilla.org/mozilla-central/rev/4236645e6bdf

Approval Request Comment [Feature/regressing bug #]: Bug 1230462 [User impact if declined]: For some user the http authentication dialog can be cut. Does not look good, see attached images. [Describe test coverage new/current, TreeHerder]: This bug only change the text that we show, it only removes new lines. It was tested by a user with this problem, comment #25 [Risks and why]: none [String/UUID change made/needed]: none

Comment on attachment 8792492 [details] [diff] [review] bug_1277895.patch rebased for aurora This missed the uplift.

(In reply to Dragana Damjanovic [:dragana] from comment #44) > [String/UUID change made/needed]: none This is adding a new string in toolkit/locales/en-US/chrome/global/commonDialogs.properties, isn't it?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #46) > (In reply to Dragana Damjanovic [:dragana] from comment #44) > > [String/UUID change made/needed]: none > > This is adding a new string in > toolkit/locales/en-US/chrome/global/commonDialogs.properties, isn't it? :) Yes it does. Thanks.

It's adding 3 strings, and I'm definitely against uplifting this kind of patch to a string frozen branch: dialog being cut is better than the dialog being displayed in a language users don't understand. Alternative solution: strip newline characters from existing strings in beta (it will need a beta-only patch).

Hi Dragana, I am on the same page as Flod on this one. Can we do the alternative solution he is proposing? Or should we wontfix this for 50? I see the visual regression but I think we can live with it.

(In reply to Ritu Kothari (:ritu) from comment #49) > Hi Dragana, I am on the same page as Flod on this one. Can we do the > alternative solution he is proposing? Or should we wontfix this for 50? I > see the visual regression but I think we can live with it. I will make a patch that strips newlines, and lets see if it is easy or not, I can at least do it in some languages. flod, can you tell me where are the strings. Thanks

I thought that flod was suggesting to strip the newlines at runtime e.g. with a regex replacement.

(In reply to Matthew N. [:MattN] (In Taipei until Sep. 23) from comment #51) > I thought that flod was suggesting to strip the newlines at runtime e.g. > with a regex replacement. Exactly.

Hello Dragana, any luck getting the Beta patch ready? Thanks!

(In reply to Francesco Lodolo [:flod] from comment #52) > (In reply to Matthew N. [:MattN] (In Taipei until Sep. 23) from comment #51) > > I thought that flod was suggesting to strip the newlines at runtime e.g. > > with a regex replacement. > > Exactly. can you hlp me with this, just the hit where the fix should go, I do not know this code, I work on necko :( Thanks!

(In reply to Dragana Damjanovic [:dragana] from comment #54) > can you hlp me with this, just the hit where the fix should go, I do not > know this code, I work on necko :( I don't think I'm the right person to help, I work on localization and barely touch code. The idea would be to use a regexp to clean up text, e.g. text.replace(/\n{1,}/g, ' ') (or probably something smarter).

Comment on attachment 8799447 [details] [diff] [review] bug_1277895_beta.patch Review of attachment 8799447 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/components/PromptService.js @@ +705,5 @@ > } else { > text = this.bundle.formatStringFromName("EnterLoginForRealm2", [realm, displayHost], 2); > } > > + return text.replace(/\n{1,}/g,' '); Nit: you're missing the space after the comma which is required per the Mozilla style guide. ::: toolkit/components/prompts/src/nsPrompter.js @@ +284,5 @@ > } else { > text = PromptUtils.getLocalizedString("EnterLoginForRealm2", [realm, displayHost]); > } > > + return text.replace(/\n{1,}/g,' '); Ditto

To clarify, I mean before the 2nd argument.

Hi Dragana, ping to bump this up in your to-fix queue. :)

(In reply to Ritu Kothari (:ritu) from comment #59) > Hi Dragana, ping to bump this up in your to-fix queue. :) I make a patch sometime ago. I would like dolske to take a look/review. I will make an update tomorrow to incorporate comment 57, but that is just a style issue.

Comment on attachment 8799447 [details] [diff] [review] bug_1277895_beta.patch Review of attachment 8799447 [details] [diff] [review]: ----------------------------------------------------------------- Nit: please improve the commit message to describe the solution, not the problem. e.g. Bug 1277895 - Remove new line characters from HTTP auth dialogs to avoid layout issues. r=MattN I'm not as familiar with this as dolske but in the interest of getting this fixed on Beta I'll review it anyways (since I know dolske is traveling this week). The patch seems reasonable to me. ::: mobile/android/components/PromptService.js @@ +705,5 @@ > } else { > text = this.bundle.formatStringFromName("EnterLoginForRealm2", [realm, displayHost], 2); > } > > + return text.replace(/\n{1,}/g,' '); Does Android even need this change? I would have expected native dialogs there but I don't know enough about this.

(In reply to Matthew N. [:MattN] from comment #61) > Comment on attachment 8799447 [details] [diff] [review] > bug_1277895_beta.patch > > Review of attachment 8799447 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nit: please improve the commit message to describe the solution, not the > problem. > e.g. Bug 1277895 - Remove new line characters from HTTP auth dialogs to > avoid layout issues. r=MattN > > I'm not as familiar with this as dolske but in the interest of getting this > fixed on Beta I'll review it anyways (since I know dolske is traveling this > week). The patch seems reasonable to me. > > ::: mobile/android/components/PromptService.js > @@ +705,5 @@ > > } else { > > text = this.bundle.formatStringFromName("EnterLoginForRealm2", [realm, displayHost], 2); > > } > > > > + return text.replace(/\n{1,}/g,' '); > > Does Android even need this change? I would have expected native dialogs > there but I don't know enough about this. I am not sure. I do not know this code at all. To be sure I will remove the new lines

Approval Request Comment [Feature/regressing bug #]: Bug 1230462 [User impact if declined]: Http authentication dialog can be cut, see attached build cut-auth-dialog-1.png [Describe test coverage new/current, TreeHerder]: [Risks and why]: low [String/UUID change made/needed]: none

Comment on attachment 8802023 [details] [diff] [review] bug_1277895_beta.patch Fixes a recent regression, Beta50+

This is actually a more general problem relating to a rounding error when computing the checkbox length on Windows. See bug 1281616.