1148807 - Method moveToAlertPosition in dialog.xml should check if opener is not null.

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Philip Chee]

(From Bug 1134891 comment #29)
> (In reply to Philip Chee from comment #26)
>> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)

>>> I think we should still take this for anything else that might use
>>> dialogOverlay, but we need to make it so that the dialogs are all being
>>> opened such that the parent is the browser window, and not the content
>>> window for what's being printed.
>> 
>> Does this implementation of moveToAlertPosition() need to be fixed as well?
>> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/
>> dialog.xml?rev=0c9ed31fcb96#98
> 
> Only if the dialog in question ever gets called with a content opener... Can
> you file a followup bug to investigate the remaining callers and assign to
> me? It's hard to tell from MXR whether the opener will always be OK or not...

It's not just the in-tree consumers but the whole add-ons ecosystem. Since it's impossible to audit all callers we might as well add the check to moveToAlertPosition() and be done with it.

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: bz://1148807/Gijs

/r/6413 - Bug 1148807 - fix dialog.xml to deal with a null opener, too - belt+suspenders fix, r?mconley

Pull down this commit:

hg pull -r d5b70c98a2249c5009e17fa1f3a23d78bfad3b03 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[:Gijs (he/him)]

This is necessary followup from bug 1115333 and/or bug 1134891, and also trivial.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8586427 [details]
MozReview Request: bz://1148807/Gijs

https://reviewboard.mozilla.org/r/6411/#review5367

::: toolkit/content/widgets/dialog.xml
(Diff revision 1)
> +            newX = (screen.availWidth - window.outerWidth) / 2;
> +            newY = (screen.availHeight - window.outerHeight) / 2;

Hey Gijs - just double-checking here... we're OK that newX and newY are not being declared in this conditional branch as var? Is this some wacky hoisting behaviour that var has? I haven't used var in so long that I'm not sure what is valid or invalid here.

Comment 4

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> Comment on attachment 8586427 [details]
> MozReview Request: bz://1148807/Gijs
> 
> https://reviewboard.mozilla.org/r/6411/#review5367
> 
> ::: toolkit/content/widgets/dialog.xml
> (Diff revision 1)
> > +            newX = (screen.availWidth - window.outerWidth) / 2;
> > +            newY = (screen.availHeight - window.outerHeight) / 2;
> 
> Hey Gijs - just double-checking here... we're OK that newX and newY are not
> being declared in this conditional branch as var? Is this some wacky
> hoisting behaviour that var has? I haven't used var in so long that I'm not
> sure what is valid or invalid here.

The basic rule with var is pretty simple: you need it exactly once per function for any variable. The declaration part gets hoisted, the assignment happens where you put the statement. So yes, this works.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

var is so screwy.

Comment 6

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/7ab84d5897f5

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/7ab84d5897f5

Comment 9

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1148807 - fix dialog.xml to deal with a null opener, too - belt+suspenders fix, r?mconley