HTTP auth dialog gets cut off in certain circumstances

RESOLVED FIXED in Firefox 50

Status

()

Toolkit
Notifications and Alerts
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

({regression})

Trunk
mozilla51
regression
Points:
---

Firefox Tracking Flags

(firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, firefox50+ fixed, firefox51 fixed)

Details

Attachments

(5 attachments, 5 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8759724 [details]
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.
(Assignee)

Comment 1

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

Comment 2

2 years ago
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.
Component: General → Notifications and Alerts
Product: Firefox → Toolkit
Summary: Prompt dialog is cut → HTTP auth dialog gets cut off in certain circumstances
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?
Blocks: 1230462
Flags: needinfo?(honzab.moz)
Keywords: regression
(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
Flags: needinfo?(honzab.moz)
[Tracking Requested - why for this release]: Ugly visual regression.
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
status-firefox50: --- → affected
tracking-firefox50: --- → ?
I see this on Win10 all the time when connecting to https://secure.pub.build.mozilla.org/buildapi/

Updated

2 years ago
See Also: → bug 1281616
Tracking 50+ for this visible regression.
tracking-firefox50: ? → +
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.
Flags: needinfo?(enndeakin)
...if I make one of the dialog buttons actually just call window.sizeToContent() when clicked, it still doesn't do anything.

Updated

2 years ago
Duplicate of this bug: 1284040
Version: unspecified → Trunk
status-firefox47: --- → unaffected
Do we have a plan for addressing this regression?
Flags: needinfo?(dolske)
(Assignee)

Comment 12

2 years ago
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

2 years ago
I'm unable to reproduce this so I can't be much help, sorry.
Flags: needinfo?(enndeakin)
(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.
Flags: needinfo?(dolske) → needinfo?(dd.mozilla)
(Assignee)

Comment 15

2 years ago
Created attachment 8781915 [details] [diff] [review]
bug_1277895.patch
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(dd.mozilla)
Attachment #8781915 - Flags: review?(dolske)
(Assignee)

Comment 17

2 years ago
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.
(Assignee)

Comment 18

2 years ago
(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
(Assignee)

Comment 19

2 years ago
Honza, can you try a build from comment #16. I have removed an extra line break. Thanks!!!
Flags: needinfo?(honzab.moz)
Created attachment 8782054 [details]
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 :(((
Flags: needinfo?(honzab.moz)
Unfortunately, I'd suspect the issue here is with _any_ line break, so fixing this would remove both of them from the string.
(Assignee)

Comment 22

2 years ago
Created attachment 8782148 [details] [diff] [review]
bug_1277895.patch

Now without both new lines.
Attachment #8781915 - Attachment is obsolete: true
Attachment #8781915 - Flags: review?(dolske)
Attachment #8782148 - Flags: review?(dolske)
(Assignee)

Comment 24

2 years ago
Honza, can you try the build from comment 23.
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
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.
Attachment #8782148 - Flags: review?(dolske) → review+
(Assignee)

Comment 27

2 years ago
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
Flags: needinfo?(honzab.moz)
(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... :(
Flags: needinfo?(honzab.moz)

Comment 29

2 years ago
(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.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 31

2 years ago
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.
Flags: needinfo?(honzab.moz)
(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 :(
Flags: needinfo?(honzab.moz)
Attachment #8785957 - Attachment is obsolete: true
Needs rebasing.
status-firefox51: --- → affected
Keywords: checkin-needed
(Assignee)

Comment 35

2 years ago
Created attachment 8791082 [details] [diff] [review]
bug_1277895.patch

rebased.
Attachment #8782148 - Attachment is obsolete: true
Attachment #8791082 - Flags: review+
(Assignee)

Comment 37

2 years ago
Created attachment 8791377 [details] [diff] [review]
bug_1277895.patch
Attachment #8791082 - Attachment is obsolete: true
Attachment #8791377 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
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
Flags: needinfo?(dd.mozilla)
Keywords: checkin-needed
(Assignee)

Comment 39

2 years ago
(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.
Flags: needinfo?(dd.mozilla)

Comment 42

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/88570c6adbe2
https://hg.mozilla.org/mozilla-central/rev/4236645e6bdf
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 44

2 years ago
Created attachment 8792492 [details] [diff] [review]
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
Attachment #8792492 - Flags: approval-mozilla-aurora?
Comment on attachment 8792492 [details] [diff] [review]
bug_1277895.patch rebased for aurora

This missed the uplift.
Attachment #8792492 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
(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?
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 47

2 years ago
(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.
Flags: needinfo?(dd.mozilla)
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.
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 50

2 years ago
(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
Flags: needinfo?(dd.mozilla) → needinfo?(francesco.lodolo)
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.
Flags: needinfo?(francesco.lodolo)

Updated

2 years ago
Attachment #8792492 - Flags: approval-mozilla-beta?
Hello Dragana, any luck getting the Beta patch ready? Thanks!
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 54

2 years ago
(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!
Flags: needinfo?(dd.mozilla) → needinfo?(francesco.lodolo)
(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).
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 56

2 years ago
Created attachment 8799447 [details] [diff] [review]
bug_1277895_beta.patch
Attachment #8799447 - Flags: review?(dolske)
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. :)
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 60

2 years ago
(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.
Flags: needinfo?(dd.mozilla)
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.
Attachment #8799447 - Flags: review?(dolske) → review+
(Assignee)

Comment 62

2 years ago
(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
(Assignee)

Comment 63

2 years ago
Created attachment 8802023 [details] [diff] [review]
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
Attachment #8799447 - Attachment is obsolete: true
Attachment #8802023 - Flags: review+
Attachment #8802023 - Flags: approval-mozilla-beta?
Comment on attachment 8802023 [details] [diff] [review]
bug_1277895_beta.patch

Fixes a recent regression, Beta50+
Attachment #8802023 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 65

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2bcf7a0d6dd0
status-firefox50: affected → fixed

Comment 66

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

See bug 1281616.
You need to log in before you can comment on or make changes to this bug.