1277895 - HTTP auth dialog gets cut off in certain circumstances

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Dragana Damjanovic [:dragana]]

Attachment: cut-auth-dialog-1.png

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.

Comment 1

[Dragana Damjanovic [:dragana]]

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

Comment 2

[:Gijs (he/him)]

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.

Comment 3

[Justin Dolske [:Dolske]]

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?

Comment 4

[Honza Bambas (:mayhemer)]

(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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Marcia Knous [:marcia]]

Tracking 50+ for this visible regression.

Comment 8

[Justin Dolske [:Dolske]]

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.

Comment 9

[Justin Dolske [:Dolske]]

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

Comment 11

[Andrew Overholt [:overholt]]

Do we have a plan for addressing this regression?

Comment 12

[Dragana Damjanovic [:dragana]]

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 :)

Comment 13

[Neil Deakin]

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

Comment 14

[Justin Dolske [:Dolske]]

(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.

Comment 15

[Dragana Damjanovic [:dragana]]

Attachment: bug_1277895.patch

Comment 16

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb999db5f92e

Comment 17

[Dragana Damjanovic [:dragana]]

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.

Comment 18

[Dragana Damjanovic [:dragana]]

(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

Comment 19

[Dragana Damjanovic [:dragana]]

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

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: authdiag-still-crop.png

(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 :(((

Comment 21

[Justin Dolske [:Dolske]]

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

Comment 22

[Dragana Damjanovic [:dragana]]

Attachment: bug_1277895.patch

Now without both new lines.

Comment 23

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d11b296f0b44

Comment 24

[Dragana Damjanovic [:dragana]]

Honza, can you try the build from comment 23.

Comment 25

[Honza Bambas (:mayhemer)]

(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 26

[Justin Dolske [:Dolske]]

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.

Comment 27

[Dragana Damjanovic [:dragana]]

Attachment: 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

Comment 28

[Honza Bambas (:mayhemer)]

(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... :(

Comment 29

[:Paolo Amadini]

(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.

Comment 30

[Justin Dolske [:Dolske]]

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.

Comment 31

[Dragana Damjanovic [:dragana]]

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.

Comment 32

[Honza Bambas (:mayhemer)]

(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 :(

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 34

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cdebc8e91a5

Comment 35

[Dragana Damjanovic [:dragana]]

Attachment: bug_1277895.patch

rebased.

Comment 36

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b6c6c604b0

Comment 37

[Dragana Damjanovic [:dragana]]

Attachment: bug_1277895.patch

Comment 38

[Carsten Book [:Tomcat]]

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

Comment 39

[Dragana Damjanovic [:dragana]]

(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.

Comment 40

[Dragana Damjanovic [:dragana]]

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

Comment 41

[Dragana Damjanovic [:dragana]]

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

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 43

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bd9ab4d55ed

Comment 44

[Dragana Damjanovic [:dragana]]

Attachment: bug_1277895.patch rebased for aurora

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 45

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8792492 [details] [diff] [review]
bug_1277895.patch rebased for aurora

This missed the uplift.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

(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?

Comment 47

[Dragana Damjanovic [:dragana]]

(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.

Comment 48

[Francesco Lodolo [:flod]]

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).

Comment 49

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 50

[Dragana Damjanovic [:dragana]]

(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

Comment 51

[Matthew N. [:MattN]]

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

Comment 52

[Francesco Lodolo [:flod]]

(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.

Comment 53

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 54

[Dragana Damjanovic [:dragana]]

(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!

Comment 55

[Francesco Lodolo [:flod]]

(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 56

[Dragana Damjanovic [:dragana]]

Attachment: bug_1277895_beta.patch

Comment 57

[Matthew N. [:MattN]]

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

Comment 58

[Matthew N. [:MattN]]

To clarify, I mean before the 2nd argument.

Comment 59

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 60

[Dragana Damjanovic [:dragana]]

(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 61

[Matthew N. [:MattN]]

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.

Comment 62

[Dragana Damjanovic [:dragana]]

(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

Comment 63

[Dragana Damjanovic [:dragana]]

Attachment: bug_1277895_beta.patch

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 64

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8802023 [details] [diff] [review]
bug_1277895_beta.patch

Fixes a recent regression, Beta50+

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2bcf7a0d6dd0

Comment 66

[Mike Kaply [:mkaply]]

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

See bug 1281616.