Closed
Bug 507622
Opened 15 years ago
Closed 11 years ago
remove some usage of obsolete dialogOverlay.xul in mailnews/
Categories
(Thunderbird :: Mail Window Front End, defect)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 23.0
People
(Reporter: mkmelin, Assigned: mkmelin)
Details
Attachments
(1 file, 3 obsolete files)
11.77 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
dialogOverlay.xul is obsolete in favor of dialog.xml Convert the few instances of using <window> as dialog. The upside is that a ok/cancel buttons get ordered correctly in a few places. The selectAddressesWindow was pretty unhappy as a dialog. Dunno why, but the dialog is blank if that includes both bundle_composeMsgs and MsgComposeCommands.js. Fortunately, don't need the latter at all. Which means we don't need the former either! abSelectAddressesDialog.xul doesn't belong in mailnews/ but i'll leave moving it to suite to another bug.
Attachment #391862 -
Flags: superreview?(neil)
Attachment #391862 -
Flags: review?(neil)
Comment 1•15 years ago
|
||
Comment on attachment 391862 [details] [diff] [review] proposed fix >+ var abPopup = document.getElementById("abPopup"); Oh dear, how long has that been missing? ;-) >+ if (abPopup) { But I'm fairly sure it exists, given that the abList exists... >-function OnUnloadSelectAddress() >-{ >- CloseAbView(); >-} This is wrong, we need to close the view whether we OK or cancel the dialog. >+ removeButton.disabled = !bucketTree.view || How can that happen? It's unusual for a tree not to have a view... > >+ <vbox flex="1"> > >- <keyset id="dialogKeys"/> >- >- <vbox flex="1"> >- An hg diff oddity? >- <!--<splitter id="bucket-splitter" collapse="before" persist="state"/>--> For a second I thought I was seeing double ;-) > <!-- we need to override the dialogs default key behavior so we can re-route enter/return > to the add button if the add button is suddenly the default action > --> > <keyset id="customHeaderDialogKeys"> > <key keycode="VK_ENTER" oncommand="enterKeyPressed();"/> > <key keycode="VK_RETURN" oncommand="enterKeyPressed();"/> > <key keycode="VK_ESCAPE" oncommand="doCancelButton();"/> So switching to a <dialog> is going to involve some recoding too... we had similar problems with the spellcheck dialog. >- <keyset id="dialogKeys"/> >- > <hbox class="box-header" id="header" > title="&importTitle.label;" > description="&importShortDesc.label;"/> > > <deck id="stateDeck" selectedIndex="0"> > <vbox class="wizard-box"> I can't see what you're replacing the keyset with, but I don't think it's worth touching this window. Unless of course you want to turn it into a <wizard/> ;-)
Attachment #391862 -
Flags: superreview?(neil)
Attachment #391862 -
Flags: superreview-
Attachment #391862 -
Flags: review?(neil)
Updated•15 years ago
|
Version: unspecified → Trunk
Comment 2•15 years ago
|
||
This bug is open but targeted for seamonkey2.0b2, which has been released a significant time ago. Please set the target milestone to an appropriate value, "---" if it has no specific target.
Assignee | ||
Updated•15 years ago
|
Target Milestone: seamonkey2.0b2 → ---
Comment 3•14 years ago
|
||
Magnus, any chance you can pick this up again and correct the patch according to Neil's nits?
Assignee | ||
Updated•11 years ago
|
Summary: remove usage of obsolete dialogOverlay.xul in mailnews/ → remove some usage of obsolete dialogOverlay.xul in mailnews/
Assignee | ||
Updated•11 years ago
|
Component: MailNews: General → Mail Window Front End
Product: SeaMonkey → Thunderbird
Assignee | ||
Comment 4•11 years ago
|
||
Not taking on the seamonkey address selector, and leaving the import dialog at least for now. The virtual folders dialog doesn't react to the defult ok/esc keys until focus changes, but that seems to be the case now too.
Attachment #391862 -
Attachment is obsolete: true
Attachment #728775 -
Flags: review?(neil)
Comment 5•11 years ago
|
||
Comment on attachment 728775 [details] [diff] [review] proposed fix, v2 >-function enterKeyPressed() >+function onOk() > { > // if the add button is currently the default action then add the text > if (gHeaderInputElement.value != "" && !gAddButton.disabled) > { > onAddHeader(); >+ return false; > } >- else >- { >- // otherwise, the default action for the dialog is the OK button >- if (!gOKButton.disabled) >- doOKButton(); >- } >-} > >-function onOk() >-{ r=me on the other dialogs, but I don't think this is going to work because onOk is called when you click the OK button, and you definitely don't want an add to happen in that case. Additionally you're messing with the <dialog>'s default button tracking. If you still want to convert this <window> to a <dialog>, you could try giving the other buttons their own dlgtype (extra1 and extra2 are spare) and then you can alter the <dialog>'s defaultButton property as needed.
Attachment #728775 -
Flags: review?(neil) → review-
Assignee | ||
Comment 6•11 years ago
|
||
Better fix
Attachment #728775 -
Attachment is obsolete: true
Attachment #731551 -
Flags: review?(neil)
Assignee | ||
Comment 7•11 years ago
|
||
Comment on attachment 731551 [details] [diff] [review] proposed fix, v3 Review of attachment 731551 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/search/content/CustomHeaders.xul @@ +50,5 @@ > + <button id="removeButton" > + label="&removeButton.label;" > + accesskey="&removeButton.accesskey;" > + dlgtype="extra2" > + oncommand="onRemoveHeader();"/> I realized these oncommands aren't neded anymore.
Comment 8•11 years ago
|
||
Comment on attachment 731551 [details] [diff] [review] proposed fix, v3 >- gOKButton = document.getElementById("ok"); >+ gOKButton = document.documentElement.getButton("accept"); No longer used, might as well remove it entirely. >+ // only update the button if we disabled state changed Nit: grammar incorrect >+ document.documentElement.defaultButton = (aDisable) ? "accept" : "extra1"; Nit: don't need the ()s >+ if (gRemoveButton.disabled) { >+ document.documentElement.defaultButton = "accept"; What's this got do do with anything?
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #8) > >+ if (gRemoveButton.disabled) { > >+ document.documentElement.defaultButton = "accept"; > What's this got do do with anything? Ys seems unnecessary now. In an earlier patch version i didn't move focus to the input field and then there was a strage state that remove was default/focused but disabled.
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #731551 -
Attachment is obsolete: true
Attachment #731551 -
Flags: review?(neil)
Attachment #731808 -
Flags: review?(neil)
Updated•11 years ago
|
Attachment #731808 -
Flags: review?(neil) → review+
Assignee | ||
Comment 11•11 years ago
|
||
http://hg.mozilla.org/comm-central/rev/54463e2e941d -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
You need to log in
before you can comment on or make changes to this bug.
Description
•