695485 - UI for handling <select> interaction

Status: RESOLVED FIXED

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

Description

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

Need a UI to handle user interaction with <select> elements on webpages

Comment 1

[Wesley Johnston (:wesj)]

Taking since I've already been looking at this. If you've got something going sriram, please holler and maybe we can combine our stuff some how.

Select elements can potentially contain optgroups with header rows, as well as support multiple selection. Android's dialogs don't support grouped headers easily. If I force them in by using a custom list adapter, I can't get multiple selection to work. As an alternative, I could attach my own listview, but then we loose a bit of native styling (i.e. the light colors behind the listview). I've been digging through the android source to see if I can add some of that back in, but it looks like its mostly using privileged apis and styles that we don't have access to.

So... I'm still digging. Those are oddball use cases to some extent. Getting the simplest options up and running was mostly done in the Prompt Service bug. Maybe I'll just land that and swing back around for these complex selects?

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Single select

I'm trying to break this up so that we can land what we want and not be caught up by some small piece that isn't working. This just enables single selection in select elements.

I have a piece for multi-select (working) and one for optgroups (in progress) coming.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Single select

Cleaned up a bit.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Multiple Select

Support for multiple select

Comment 5

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

Comment on attachment 569779 [details] [diff] [review]
Single select

Quick pre-review:

>+var ClickHelpers = {
>+    this._helpers.push(aHelper);
>+  },
>+  handleClick: function(aEvent) {

Add blank line between methods

>+let HTMLSelectElement = Ci.nsIDOMHTMLSelectElement;
>+let HTMLOptGroupElement = Ci.nsIDOMHTMLOptGroupElement;
>+let HTMLOptionElement = Ci.nsIDOMHTMLOptionElement;

These might already "Just Work" in a XUL JS file. They didn't workin a content JS file, but should in XUL JS.

>+var FormAssistant = {

>+  onClick: function(aEvent) {
>+    aEvent.preventDefault();
>+    aEvent.stopPropagation();

Shouldn't we test to make sure we are on a select element before just killing the event?

>+  getListForElement: function(aElement) {
>+    let result = {
>+      type: "prompt",

Nice time to change to "Prompt:Show"



Closer look coming, but that's the first pass

Comment 6

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

Comment on attachment 569781 [details] [diff] [review]
Multiple Select

Quick review:

>+    if (aElement.multiple) {
>+      result.buttons = [
>+        { label: "Done" },

Need to pull from properties file

This patch looks pretty good, and since it's based on the first, I'll do a closer pass soon on it too.

Comment 7

[Wesley Johnston (:wesj)]

> >+  onClick: function(aEvent) {
> >+    aEvent.preventDefault();
> >+    aEvent.stopPropagation();
> 
> Shouldn't we test to make sure we are on a select element before just
> killing the event?

I meant to mention this, but figured now is good enough. For multiple selection, I've noticed that onclick, we actually fire the click on the select, erasing any selection inside it. I put this in to see if I could easily prevent it. It didn't work and I forgot this was in there :(

It makes me wonder if we need to bring the shield back or something, to prevent clicks on the html document until we decide they should be fired. Either that, or we could kill off all our mouse events and just use the Android Gesture stuff...

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Support for Optgroups

Finally...

This gives us support for optgroups. For single select lists and "no" select lists (I put that in for ContextMenus) this is really easy. I wrote a little adapter. We have a special header row which inherits the style from headers in preference screens. Looks good!

For multiple selects, there is no version of Builder.setMultiChoiceItems that takes an Adapter. So instead we have to attach our own special view. That view apparently has problems if there is no title on the dialog (the list tries to "become" the dialog title), so I've put in a fake (empty string) title here (builder.setCustomTitle(new LinearLayout(...)) gives me the same result).

In addition, the list will also have an incorrect background color. I dug into the Android source to look and see what they do, but they're using (AFAICT) styles internal to android itself. One possible fix is to force the list background to "White". Or another is to use normal listview styles in these cases, which look fine on black. There's shots of both (as well as the title problem) at:

http://people.mozilla.com/~wjohnston/screenshots/selectdialogs/

Comment 9

[Wesley Johnston (:wesj)]

I should point out the stock browser somehow seems ok here too. Not sure if they've just written a custom dialog implementation or what. That's an alternative we can chase if we want to (getting optgroups to work correctly is likely not a top priority).

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Support for Optgroups v2

Dug into Android WebView source and noticed that I missed a call to builder.setInverseBackgroundForced which fixes our background color and title problems.

There are then some color problems while scrolling (the list turns black). I've imported an internal android xml file to use to fix that (which I assume is fixed by setting cachecolorHint to null). That file has a big Apache license at the top because I'm not sure what the rules are about us borrowing it?

Screenshots are updated

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Single selects v2

Slightly nicer version addressing nits and cleaning up the click handling somewhat. Still have problems with mousedown firing when I don't want it to, but other than that is fine.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Multiple Select v2

This has almost no changes except moving "Done" to browser.properties.

Comment 13

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

Comment on attachment 570134 [details] [diff] [review]
Multiple Select v2

>diff --git a/embedding/android/PromptService.java b/embedding/android/PromptService.java
>+                Log.i("GeckoShell", "Have a list");

It seems like we use a file level constant for "GeckoShell" called LOG_FILE (see some other Java files)

>+    private boolean[] getBooleanArray(JSONObject aObject, String aName) {

>+        try {
>+            items = aObject.getJSONArray(aName);
>+        } catch(Exception ex) {
>+        }

          } catch(Exception ex) { }

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>diff --git a/mobile/locales/en-US/chrome/browser.properties b/mobile/locales/en-US/chrome/browser.properties

> #Text Selection
> selectionHelper.textCopied=Text copied to clipboard
>+selectHelper.closeMultipleSelectDialog=Done
>\ No newline at end of file

Add a comment for "Select UI" and add a new line at end of file

Comment 14

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

Comment on attachment 569849 [details] [diff] [review]
Support for Optgroups v2


>diff --git a/embedding/android/PromptService.java b/embedding/android/PromptService.java
>-        String[] menuitems = getStringArray(geckoObject, "listitems");
>+        PromptListItem[] menuitems = getListItemArray(geckoObject, "listitems");

Do we still use "getStringArray" ?

>+    private PromptListItem[] getListItemArray(JSONObject aObject, String aName) {

>+        try {
>+            items = aObject.getJSONArray(aName);
>+        } catch(Exception ex) {
>+        }

On one line

>diff --git a/embedding/android/resources/layout/select_dialog_list.xml b/embedding/android/resources/layout/select_dialog_list.xml

We should check to see if this license is OK. Or you could recreate the XML file from scratch since it's not long.

These patches have a lot of code I have never used, but it looks like it should work :)
Landing and testing will tell for sure.

Comment 15

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/projects/birch/rev/ec7d971fc82c

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Missing file

Forgot to hg add this file...

Comment 17

[Wesley Johnston (:wesj)]

pushed fix:
https://hg.mozilla.org/projects/birch/rev/93a133deb97f