Closed Bug 255270 Opened 20 years ago Closed 20 years ago

[FIXr]Unchecking "Alert Me" in Security warning dialog hides Continue/Cancel buttons

Categories

(SeaMonkey :: General, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.8alpha3

People

(Reporter: pkwarren, Assigned: bzbarsky)

References

Details

(Keywords: regression, smoketest)

Attachments

(2 files, 2 obsolete files)

Steps to reproduce 1) Create a new profile. Start Mozilla. 2) Go to any page which has a form (e.g. http://www.mozilla.org/). 3) Type something in the form, and submit it. This should bring up the security warning dialog. 4) Uncheck the "Alert me" checkbox in the dialog. Notice how the label resizes and pushes the Continue/Cancel buttons off screen so they are no longer visible. This started occurring with nightly builds between 2004-08-09 and 2004-08-10. Sorry if this is a duplicate - I didn't see this reported anywhere today.
I have reproduced this problem on Windows, Linux, and AIX.
Summary: Security warning dialog resizes and hides Continue/Cancel buttons → Unchecking "Alert Me" in Security warning dialog resizes and hides Continue/Cancel buttons
Summary: Unchecking "Alert Me" in Security warning dialog resizes and hides Continue/Cancel buttons → Unchecking "Alert Me" in Security warning dialog hides Continue/Cancel buttons
> between 2004-08-09 and 2004-08-10. What actual builds? Morning? Evening?
Or more to the point, what are the timestamps on those builds?
If it's 08:00:00 to 08:00:00, this is probably a regression from bug 230170 (though I'm not sure why quite yet).
These are from nightly builds between 2004-08-09 08:00 and 2004-08-10 08:00, Pacific time.
Short story: this is a known bug in nsBoxToBlockAdaptor. I have no idea why we've suddenly started triggering it here....
I did a bit of digging about... Apparently, the change to the "collapsed" attribute of the checkbox container in the dialog is being unhappy. If I make sure to process the restyle synchronously for "collapsed" changes, things work again. By "is being unhappy" I mean that at some point we end up in a state where GetBounds() on a BoxToBlockAdaptor gives a zero-width rect. Then the incremental reflow spawned by the "checked" change (because _all_ attr changes cause reflow in XUL!) starts with a width of 0 and ends up with the minimal width it can have (which means putting each word on a separate line). I tested opening up the prefs dialog to make sure the checkbox XBL binding is preloaded, and that does not affect the problem at hand, so it looks to me like this is a problem with XUL incremental reflow, pure and simple. Apparently the reason this didn't happen before is that we changed the visibility in onload, so the initial reflow subsumed our incremental style change reflow or something... I can special-case the "collapsed" attribute if driven to it, but I think the right thing to do is to try and hunt down this XUL reflow bug...
Blocks: 255352
This is happening in alert dialogs with check boxes all over the app in trunk builds. It's affecting firefox trunk too.
Severity: normal → critical
*** Bug 255058 has been marked as a duplicate of this bug. ***
Keywords: regression
Severity: critical → major
Per Asa, made this a smoketest blocker and blocking1.8a3
Severity: major → blocker
Flags: blocking1.8a3+
Keywords: smoketest
Attached file Minimal testcase for the reflow bustage (obsolete) (deleted) —
Note that this does not use collapsed or anything. It may be that the reason the "collapsed" thing helps is that it marks descendants dirty....
Attached file Even more minimal (obsolete) (deleted) —
Attached file Per Neil's advice, more minimal yet. (deleted) —
Attachment #155966 - Attachment is obsolete: true
Attachment #155972 - Attachment is obsolete: true
Attached patch Proposed patch (deleted) — Splinter Review
Comment on attachment 155988 [details] [diff] [review] Proposed patch David, the short story is that the !needsReflow codepath doesn't do a reflow and never sets aDesiredSize.mMaximumWidth. Later on, if we have NS_REFLOW_CALC_MAX_WIDTH set, we use aDesiredSize.mMaximumWidth as our preferred width. So we come out with a preferred width of zero on incremental reflow, and then end up using our minimum width... The other option is to cache the "last used" mMaximumWidth and use it when needsReflow is false. But I'm not sure that wouldn't leave in other incremental reflow bugs... The nsHelperAppDlg.xul parts of the patch are just removing hacks that were added to sort of work around bug 189076 (which this patch fixes).
Attachment #155988 - Flags: superreview?(dbaron)
Attachment #155988 - Flags: review?(dbaron)
Assignee: general → bzbarsky
Blocks: 189076
Priority: -- → P1
Summary: Unchecking "Alert Me" in Security warning dialog hides Continue/Cancel buttons → [FIX]Unchecking "Alert Me" in Security warning dialog hides Continue/Cancel buttons
Target Milestone: --- → mozilla1.8alpha3
*** Bug 255455 has been marked as a duplicate of this bug. ***
Comment on attachment 155988 [details] [diff] [review] Proposed patch r+sr=dbaron, although you might want someone else to review the xul change
Attachment #155988 - Flags: superreview?(dbaron)
Attachment #155988 - Flags: superreview+
Attachment #155988 - Flags: review?(dbaron)
Attachment #155988 - Flags: review+
Comment on attachment 155988 [details] [diff] [review] Proposed patch Neil, could you review the XUL changes? Could this please be approved for 1.8a3? Also, I probably can't check this in until Monday.
Attachment #155988 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #155988 - Flags: review+
Attachment #155988 - Flags: approval1.8a3?
Attachment #155988 - Flags: review?(neil.parkwaycc.co.uk) → review+
Summary: [FIX]Unchecking "Alert Me" in Security warning dialog hides Continue/Cancel buttons → [FIXr]Unchecking "Alert Me" in Security warning dialog hides Continue/Cancel buttons
Comment on attachment 155988 [details] [diff] [review] Proposed patch >--- embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul 18 Apr 2004 22:00:48 -0000 1.23 >+++ embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul 12 Aug 2004 23:26:22 -0000 >@@ -68,24 +68,19 @@ > <image id="contentTypeImage"/> > </vbox> > </hbox> > > <separator orient="horizontal" class="thin"/> > > <radiogroup id="mode" oncommand="dialog.toggleChoice()" align="stretch"> > <hbox> >- <!-- XXX Wallpaper over incremental reflow bug in XUL by giving >- ourselves more space... In fact, all this business >- with the hboxes is wallpaper. --> > <radio id="useSystemDefault" >- flex="7" > label="&useSystemDefault.noDesc.label;" > accesskey="&useSystemDefault.accesskey;"/> >- <spacer flex="3"/> > </hbox> I'd advise against getting rid of the spacer altogether, radiobuttons look kind of bad on gtk at least if you let them flex horizontally as much as they want (since the hover effect is on the whole button + label area). I'd recommend just restoring this to how it was before you added the wallpaper: <radio id="useSystemDefault" flex="1" label="&useSystemDefault.noDesc.label;" accesskey="&useSystemDefault.accesskey;"/> <spacer flex="1"/>
Comment on attachment 155988 [details] [diff] [review] Proposed patch <bryner> Asa: does bz need approval for his smoketest blocker fix? http://bugzilla.mozilla.org/show_bug.cgi?id=255270 <Asa> no <Asa> a=asa
Attachment #155988 - Flags: approval1.8a3? → approval1.8a3+
I checked this in so we can get the tree open. I just restored the nsHelperAppDlg.xul code to its previous state as I said in comment 19... feel free to change it if you want to later.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
>>- flex="7" >I'd advise against getting rid of the spacer altogether, radiobuttons look kind >of bad on gtk at least if you let them flex horizontally as much as they want Except he isn't letting it flex as much as it wants, is he...
Comment on attachment 155988 [details] [diff] [review] Proposed patch Brian, I do know why that code was there, and I do know what I'm doing in removing it, as Neil pointed out. Note that the change _you_ made actually makes the rendering of the helper app dialog worse. Re-requesting approval for the patch as it should go in...
Attachment #155988 - Flags: approval1.8a3+ → approval1.8a3?
No longer blocks: 255352
*** Bug 255352 has been marked as a duplicate of this bug. ***
Comment on attachment 155988 [details] [diff] [review] Proposed patch I approved this online.
Attachment #155988 - Flags: approval1.8a3? → approval1.8a3+
I updated nsHelperAppDlg.xul as per the original patch.
*** Bug 255944 has been marked as a duplicate of this bug. ***
*** Bug 256352 has been marked as a duplicate of this bug. ***
Blocks: 198346
Product: Browser → Seamonkey
Blocks: 230170
The "security warning dialog" before submitting something to the web is gone in Seamonkey 2. VERIFIED
Status: RESOLVED → VERIFIED
(In reply to comment #29) > The "security warning dialog" before submitting something to the web is gone in > Seamonkey 2. VERIFIED Actually it isn't, but it's off by default. Maybe we should enable show_once?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: