953272 - Prompt.jsm: Using setMultiChoiceItems causes button to always return false regardless of the button

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Mike Kaply [:mkaply]]

I'm using this code:

  var p = new Prompt({
    title: "Enter your username and password",
    buttons: ["Add", "Remove"]
  });
  var items = [{label: "FOO"}];
  p = p.setMultiChoiceItems(items);
  p.show(function(data) {
    Services.prompt.alert(null, "", JSON.stringify(data));
  });

Whether I click on Add or Remove, the data has "button": [false]

If I remove the setMultiChoiceItems, I get the correct data for the button.

Comment 1

[Mike Kaply [:mkaply]]

So it looks like the button array is reused for the multichoice as well as the dialog buttons.

This means you can't have a prompt that allows you to check a couple items and then do something with those items.

That's the core bug here.

There should be a separate array for the multichoice items.

Comment 2

[Josh Dover [:jdover]]

Attachment: Add selected attribute for prompts with multichoice.

To support being able to detect which button was pressed in addition to which items were selected in the case of setMultiChoiceItems, I added a "selected" attribute to the callback argument that will present only on prompts that have multi choice items:

var p = new Prompt({
  title: "Enter your username and password",
  buttons: ["Add", "Remove"]
});
p = p.setMultiChoiceItems([{label: "FOO"}, {label: "BAR"}]);

// User selects "FOO" option and taps "Add" button

p.show(function(data) {
  // data would be: { "guid":"XXX", "selected": [true, false], "button": 0 }
});

This does not change behavior for setSingleChoiceItems which continues to assume that you will NOT have both a list of items AND buttons:

var p = new Prompt({
  title: "Enter your username and password"
});
p = p.setSingleChoiceItems([{label: "FOO"}, {label: "BAR"}]);

// User taps "FOO"

p.show(function(data) {
  // data would be: { "guid":"XXX", "button": 0 }
});

Comment 3

[Mike Kaply [:mkaply]]

Comment on attachment 8357516 [details] [diff] [review]
Add selected attribute for prompts with multichoice.

This looks like a good idea to me.

Although my gut says we should add the same thing to singlechoiceitem as well (we can leave the button there as well for backwards compatibility.

There should be some consistency between the two.

Comment 4

[Josh Dover [:jdover]]

(In reply to Mike Kaply (:mkaply) from comment #3)
> Although my gut says we should add the same thing to singlechoiceitem as
> well (we can leave the button there as well for backwards compatibility.
> 
> There should be some consistency between the two.

Good thinking, I actually now think it would better to instead of "selected" being an array of all the true/false values for each list item, to it being an array of indexes of the selected items:

  { "selected": [true, false, true] }  --->   { "selected": [0,2] }

Comment 5

[Wesley Johnston (:wesj)]

I'd be fine with that. Most of our other "input" types actually use the input name (or an id) to refer to their value) i.e. if you add a textbox you'll get:

{ textbox0: "Value" }

if you add a textbox with an id (prompt.addTextbox({id: "foo"}); that will be used:

{ foo: "Value" }

Also, you'll have to update our select element helper to handle the new api:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectHelper.js

In that case, we care not just about what was selected, but also what as unselected, which is why we have the huge boolean array going on (but your API could still work with that pretty easily).

I don't think we have enough consumers of these that I'm super worried about backwards compatibity, but we should advertise the change anyway, so that add-on authors know.

Comment 6

[Josh Dover [:jdover]]

Attachment: Add selected attribute for prompts with multichoice.

Comment 7

[Josh Dover [:jdover]]

When should I update the MDN documentation / is there any other way we should advertise this change?

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8357939 [details] [diff] [review]
Add selected attribute for prompts with multichoice.

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

Looks good!

::: mobile/android/base/prompts/Prompt.java
@@ +187,5 @@
> +                // Mirror the selected array from multi choice for consistency.
> +                JSONArray selected = new JSONArray();
> +                selected.put(which);
> +                result.put("list", selected);
> +                // Make the button be the index of the select item.

Maybe a follow up, but I think we should probably make single select lists not auto-dismiss if they have buttons on them. You mind filing that?

::: mobile/android/chrome/content/SelectHelper.js
@@ +49,3 @@
>          let i = 0;
>          this.forOptions(aElement, function(aNode) {
> +          if (aNode.selected && selected.indexOf(i) == -1) {

Can we do:

if (aNode.selected == (selected.indexOf(i) == -1)

Comment 9

[Josh Dover [:jdover]]

Attachment: Add selected attribute for prompts with multichoice.

Comment 10

[Josh Dover [:jdover]]

(In reply to Wesley Johnston (:wesj) from comment #8)
> Comment on attachment 8357939 [details] [diff] [review]
> Add selected attribute for prompts with multichoice.
> 
> Review of attachment 8357939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: mobile/android/base/prompts/Prompt.java
> @@ +187,5 @@
> > +                // Mirror the selected array from multi choice for consistency.
> > +                JSONArray selected = new JSONArray();
> > +                selected.put(which);
> > +                result.put("list", selected);
> > +                // Make the button be the index of the select item.
> 
> Maybe a follow up, but I think we should probably make single select lists
> not auto-dismiss if they have buttons on them. You mind filing that?

Filed: bug 959742.

> 
> ::: mobile/android/chrome/content/SelectHelper.js
> @@ +49,3 @@
> >          let i = 0;
> >          this.forOptions(aElement, function(aNode) {
> > +          if (aNode.selected && selected.indexOf(i) == -1) {
> 
> Can we do:
> 
> if (aNode.selected == (selected.indexOf(i) == -1)

Done.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Possible to test this?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/032a3ba018db

Comment 13

[Kevin Brosnan [Ex-Mozilla]]

This should be backed out unless the fix for the blocking bugs are trivial. This has broken basic web forms.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Backed out:
https://hg.mozilla.org/mozilla-central/rev/253b45002d73

Now we can fix it in a less stressed way. And add a few tests?

Comment 15

[Josh Dover [:jdover]]

> ::: mobile/android/chrome/content/SelectHelper.js
> @@ +49,3 @@
> >          let i = 0;
> >          this.forOptions(aElement, function(aNode) {
> > +          if (aNode.selected && selected.indexOf(i) == -1) {
> 
> Can we do:
> 
> if (aNode.selected == (selected.indexOf(i) == -1)

This logic change was what broke the selects in this change. Reverting to previous logic fixes the issue. Marking new patch as obsolete and removing obsolete flag from previous patch. Working on tests.

Comment 16

[Josh Dover [:jdover]]

Comment on attachment 8359932 [details] [diff] [review]
Add selected attribute for prompts with multichoice.

This logic change breaks selects.

Comment 17

[Josh Dover [:jdover]]

Attachment: 953272.diff

Here are the robocop tests for single and multi selects.

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 8363362 [details] [diff] [review]
953272.diff

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

Michael should review this new test stuff

::: mobile/android/base/tests/helpers/FormHelper.java
@@ +49,5 @@
> +        assertTrue("Select item is current choice", choiceFound);
> +    }
> +
> +    public static void clickLink(String name) {
> +        sSolo.clickOnWebElement(By.textContent(name));

This probably doesn't work.

@@ +52,5 @@
> +    public static void clickLink(String name) {
> +        sSolo.clickOnWebElement(By.textContent(name));
> +    }
> +
> +    private static void clickOnWebViewItem(int offset) {

You should add a note that this method is really dumb and just clicks at the top of the page + offset. Maybe a // TODO: comment about making it smarter.

::: mobile/android/base/tests/testSelect.java
@@ +11,5 @@
> +        multiSelect();
> +    }
> +
> +    public void singleSelect() {
> +        FormHelper.openSelect(0);

I think I'd rather this api was just FormHelper.clickAt(0,0); until we have a better way of doing this.

@@ +12,5 @@
> +    }
> +
> +    public void singleSelect() {
> +        FormHelper.openSelect(0);
> +        // Waits for title to change

This seems likely to be racy. I think you'd be better to just wait for the text "Item 2". Then click it.

@@ +19,5 @@
> +            public void run() {
> +                FormHelper.chooseSingleSelectItem("Item 2");
> +            }
> +        });
> +        mToolbar.assertTitle("single2");

It also scares me this would be racy. i.e. if the next test starts and flips the title before this fires, we'd hit problems. I'd rather be explicit here. And do something like.

NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_SELECT_URL + "#single");
// The page would have to check its location and choose what to do based on that.
FormHelper.clickAt(0, 0);

FormHelper.select("Item 2");
  // This would do a waitForText(txt); clickOnText(txt);

// Maybe we could add these in a different patch...
// FormHelper.assertDisabled("Item 3");
// FormHelper.assertChecked("Item 1");
// FormHelper.assertGroup("Group 1");
// FormHelper.assertDisabled("Group 2");
// FormHelper.assertDisabled("Group 2 Item 1");

mToolbar.assertTitle("single2");

Comment 20

[Josh Dover [:jdover]]

(In reply to Wesley Johnston (:wesj) from comment #19)
> > +    public static void clickLink(String name) {
> > +        sSolo.clickOnWebElement(By.textContent(name));
> 
> This probably doesn't work.

You're right, I meant to remove this.

> > +    public void singleSelect() {
> > +        FormHelper.openSelect(0);
> > +        // Waits for title to change
> 
> This seems likely to be racy. I think you'd be better to just wait for the
> text "Item 2". Then click it.

I don't think it's clear from the code what this waits for. It isn't waiting for the select to popup, it blocks the current test thread after the run() block is executed, waiting for the title to change, so:

1. Click on new select choice
2. Wait for the title to change
3. Continue test (assert)

In that case, I don't think any other tests could happen before this completes, and the run() can't happen after the assert. I'm not defending that this is the best way, but until we can communicate bewteen JS and Java in the same test (that way we could do things like invoke the click with JS, make callback for a change event, etc.), I don't know how else to do this.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8363362 [details] [diff] [review]
953272.diff

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

Definitely in the right direction: it's a really good first effort at adding tests (and congratulations, you're the first besides myself to do so ^_^)!

I think we need to identify which Robotium methods work and which do not - this would be good to document for future travelers (maybe at [1]?) - and use that to clarify what this FormHelper API should be (and whether it should be a Click/WebContentHelper instead!). You can ping me if you want to talk it over.

I tend to be overly pedantic, so fix the nits that you think they are relevant and if you disagree, feel free to ping me if you're curious why I'd prefer those things done a different way.

[1]: https://wiki.mozilla.org/Mobile/Fennec/Android/UITest

::: mobile/android/base/tests/helpers/FormHelper.java
@@ +8,5 @@
> +
> +import org.mozilla.gecko.tests.UITestContext;
> +
> +import com.jayway.android.robotium.solo.Solo;
> +import com.jayway.android.robotium.solo.By;

nit: Alphabetize.

@@ +15,5 @@
> +import android.util.DisplayMetrics;
> +
> +/**
> + * Provides helper functionality for navigating around the Firefox UI. These functions will often
> + * combine actions taken on multiple components to perform larger interactions.

This seems really general - by Firefox UI, you mean the web content's form functionality, right? Also, most helpers combine actions taken on multiple components - I don't think it's necessary to state this.

@@ +26,5 @@
> +        sContext = context;
> +        sSolo = context.getSolo();
> +    }
> +
> +    public static void openSelect(int offset) {

nit: `final int offset`

finals aren't necessary, but they help and since I've done it for the already written code, I'm trying to make it consistent. :)

@@ +32,5 @@
> +        clickOnWebViewItem(offset);
> +    }
> +
> +    public static void chooseSingleSelectItem(String item) {
> +        // sSolo.waitForText(item);

Remove the commented-out lines here and above (unless you throw a TODO in there and explain why it's relevant, though the TODO comment can usually be more descriptive than commented-out code anyway).

@@ +33,5 @@
> +    }
> +
> +    public static void chooseSingleSelectItem(String item) {
> +        // sSolo.waitForText(item);
> +        sSolo.clickOnText(item);

Does this work? Looking at the Robotium source, it seems like this filters on TextView.class. If so, some of my assumptions about how Robotium works might be incorrect.

@@ +36,5 @@
> +        // sSolo.waitForText(item);
> +        sSolo.clickOnText(item);
> +    }
> +
> +    public static void chooseMutltiSelectItems(String... vars) {

nit: Spelling.

@@ +43,5 @@
> +        }
> +        sSolo.clickOnText("Done");
> +    }
> +
> +    public static void assertSelectCurrentChoice(String choice) {

Unused?

@@ +48,5 @@
> +        boolean choiceFound = sSolo.waitForText(choice);
> +        assertTrue("Select item is current choice", choiceFound);
> +    }
> +
> +    public static void clickLink(String name) {

Unused?

@@ +52,5 @@
> +    public static void clickLink(String name) {
> +        sSolo.clickOnWebElement(By.textContent(name));
> +    }
> +
> +    private static void clickOnWebViewItem(int offset) {

Better yet, perhaps this shouldn't be called clickOnWebViewItem, but rather, a method named after clicking in web content at various offsets (e.g. clickOnWebContentAtOffset). Perhaps it would assume a constant element height (commented!), and all elements in test HTML files can inherit this height (provided that's simple enough to do - in a div with constant height, perhaps?), standardizing the way web content click tests are implemented using this method.

Wes also had a good idea to use page anchors, making it so you only have to click at the top of the page which seems slightly less error prone (assuming the page is long enough to scroll so far down!). Perhaps this could be an overloaded version of this method, i.e. clickOnWebContentAtOffset(int offset, String anchorname).

nit: WebView should probably be reserved for native Android WebViews - I usually say "web content".

@@ +56,5 @@
> +    private static void clickOnWebViewItem(int offset) {
> +        DisplayMetrics dm = new DisplayMetrics();
> +        sContext.getActivity().getWindowManager().getDefaultDisplay().getMetrics(dm);
> +
> +        // The web content we are trying to open the context menu for should be positioned at the top of the page, at least 60px heigh and aligned to the middle

nit: I was wrapping to ~100 characters for UITest files and generally never go over 120 in Fennec code (but that's just me being pedantic!).

nit: height

nit: Why should the content be set up that way? Write some motivation and maybe make some constants! This also seems like a hack - you should probably mention so. :)

@@ +58,5 @@
> +        sContext.getActivity().getWindowManager().getDefaultDisplay().getMetrics(dm);
> +
> +        // The web content we are trying to open the context menu for should be positioned at the top of the page, at least 60px heigh and aligned to the middle
> +        float left = sContext.getDriver().getGeckoLeft() + sContext.getDriver().getGeckoWidth() / 2;
> +        float top = sContext.getDriver().getGeckoTop() + 30 * dm.density + offset;

nit: final float

::: mobile/android/base/tests/robocop.ini
@@ +90,5 @@
>  # Using UITest
>  [testAboutHomePageNavigation]
>  [testAboutHomeVisibility]
>  [testSessionHistory]
> +[testSelect]

nit: Alphabetize.

::: mobile/android/base/tests/robocop_select.html
@@ +1,1 @@
> +<html>

<!DOCTYPE html>

:)

@@ +1,3 @@
> +<html>
> +<meta charset="utf-8">
> +<title>Browser Form</title>

I wonder if setting a custom viewport meta tag (e.g. "device-width") [1] might be more consistent across devices here, though I think we might use a consistent viewport size for pages with an unspecified viewport so maybe not.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Mobile/Viewport_meta_tag#Viewport_basics

@@ +4,5 @@
> +<body>
> +<select id="single" style="font-size: 8em; width: 100%; ">
> +<option value="single1">Item 1</option>
> +<option value="single2">Item 2</option>
> +<option value="single3">Item 3</option>

nit: indent inside the <select> tag.

@@ +12,5 @@
> +<option value="multi1">Option 1</option>
> +<option value="multi2">Option 2</option>
> +<option value="multi3">Option 3</option>
> +</select>
> +<!-- Change the page's title so the test can easily check for the value. -->

Explain a bit more about why this is (e.g. "We cannot access Gecko from a running Robotium test, so we change the page title and check that instead."). Elaborating on motivation is helpful because if those pre-conditions change (e.g. we can access Gecko from UITest), perhaps someone will come along and fix this hacky workaround.

Which reminds me, you should mention this is a hack. :)

nit: whitespace above comment.

@@ +14,5 @@
> +<option value="multi3">Option 3</option>
> +</select>
> +<!-- Change the page's title so the test can easily check for the value. -->
> +<script type="text/javascript">
> +  var updateTitle = function(element) {

nit: I usually `function updateTitle(...) {` because anonymous functions are harder to follow in the debugger. Any reason you did it this way (I'm curious for my edification :P)!

@@ +25,5 @@
> +      document.title = title;
> +    } else {
> +      document.title = element.value;
> +    }
> +  };

nit: Add a newline below this function.

::: mobile/android/base/tests/testSelect.java
@@ +1,5 @@
> +package org.mozilla.gecko.tests;
> +
> +import org.mozilla.gecko.tests.helpers.*;
> +
> +public class testSelect extends UITest {

Javadoc comment explaining what this test does.

@@ +11,5 @@
> +        multiSelect();
> +    }
> +
> +    public void singleSelect() {
> +        FormHelper.openSelect(0);

I agree. However, if this were to be "clickOnWebContentAtOffset" or similar, this should be fine.

@@ +12,5 @@
> +    }
> +
> +    public void singleSelect() {
> +        FormHelper.openSelect(0);
> +        // Waits for title to change

I don't think Robotium's waitForText works though.

This shouldn't be racy, provided pageLoad waits for the title change event (debatable - bug 946656 - but I'm on it). However, it's inefficient because I don't believe the DOMContentLoaded is fired here and we'd have to wait on it.

You *could* leave it this way, but make sure to add a TODO and a followup bug to factor this out and wait on the specific page title change event (or a bug to add a WaitHelper.waitForTitleChange method - I realized this is better than factoring out the ToolbarTitleChangeVerifier as we discussed in person).

nit: Newline above comment.

@@ +19,5 @@
> +            public void run() {
> +                FormHelper.chooseSingleSelectItem("Item 2");
> +            }
> +        });
> +        mToolbar.assertTitle("single2");

What wes is striving here seems reasonable as far as what we can do with the API, though Josh is correct that the next assertion (note: you said "test" but I think you mean "assertion") won't occur until after we assert (i.e. no race).

I'm skeptical we can select based on text content though. Does that work, Josh?

@@ +24,5 @@
> +    }
> +
> +     public void multiSelect() {
> +        FormHelper.openSelect(200);
> +        // Waits for title to change

nit: Newline above comment.

Comment 22

[Wesley Johnston (:wesj)]

I landed this bit for now, but I'll leave this open for the tests:
https://hg.mozilla.org/integration/fx-team/rev/24b72a63ce26

Comment 23

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/24b72a63ce26

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 25

[Richard Z.]

Isn't the problem that we don't have the possibility to use "id" for buttons? Afaics every other kind of element can have an id except buttons. It would be a nice way to structure results when various dialog elements are combined.

The next little problem is that for *choiceItems the ids are defined for the single elements - it would have been nice to have an id for the whole list.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Leaving open for 5 years, I think it is time to close it.