974466 - SelectHelper passed document instead of window to prompt

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Prompt.jsm takes a window argument to prompts (for a future tab-modal-prompt world). SelectHelper passes a document instead of a window. We should fix that.

Comment 1

[:Margaret Leibovic]

Tristan, are you interested in taking a look at this? I'm not sure if it's valid anymore.

Comment 2

[Tristan]

Cheers. I'll take a look.

Comment 3

[Tristan]

Attachment: 974466.patch

Here's the code in question:

    let p = new Prompt({
      window: element.contentDocument
    });

It's wrong, but doesn't seem to cause any bugs. It's actually passing undefined. The fix is to use either of these:

    let p = new Prompt({
      window: window
    });

or...
    
    let p = new Prompt(); // also requires minor changes to Prompt.jsm to handle no argument

If we pass a reference to window then the instance of Prompt will be associated with its tab [ref 2]. The only benefit I can find is that the prompt could hide/show itself when you change tabs [ref 3], but because the prompt is modal (greys out the rest if the UI) I don't think it's possible to change tabs with it open anyway and I can't properly test it. 

I think the second approach is better, but let me know if you want to do the first.

Here are the test results for the second approach: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c4925c3c437 . It looks like somthing unrelated failed, but I'm not across the testing yet.

[1] https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectHelper.js#59
[2] https://lxr.mozilla.org/mozilla-central/source/mobile/android/modules/Prompt.jsm#24
[3] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java#123

Comment 4

[:Margaret Leibovic]

Comment on attachment 8746924 [details] [diff] [review]
974466.patch

Review of attachment 8746924 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/SelectHelper.js
@@ -56,5 @@
>    show: function(element) {
>      let list = this.getListForElement(element);
> -    let p = new Prompt({
> -      window: element.contentDocument
> -    });

I think we should actually still pass the window here, so that this is associated with the tab. You can use WebIDE to set a breakpoint and inspect the JS here to figure out what methods you need to call to get at the window, but I think what you want is `element.ownerDocument.defaultView`.

::: mobile/android/modules/Prompt.jsm
@@ +15,5 @@
>  function log(msg) {
>    Services.console.logStringMessage(msg);
>  }
>  
>  function Prompt(aOptions) {

Instead, you could use a default parameter value here, declaring this as:

function Prompt(aOptions = {}) {

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters

Comment 5

[Tristan]

Attachment: 974466.patch

This works for me, and looks right under a debugger. Thanks Margaret.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8749544 [details] [diff] [review]
974466.patch

Review of attachment 8749544 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/de0ebe1a027d

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de0ebe1a027d