UI for handling <select> interaction

RESOLVED FIXED

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: blassey, Assigned: wesj)

Tracking

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

(Whiteboard: [birch] [ux needed])

Attachments

(4 attachments, 4 obsolete attachments)

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

Updated

6 years ago
Whiteboard: [birch] [ux needed]

Updated

6 years ago
Priority: -- → P1

Updated

6 years ago
Assignee: nobody → sriram
(Assignee)

Updated

6 years ago
Assignee: sriram → wjohnston
(Assignee)

Comment 1

6 years ago
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?
(Assignee)

Comment 2

6 years ago
Created attachment 569748 [details] [diff] [review]
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.
(Assignee)

Comment 3

6 years ago
Created attachment 569779 [details] [diff] [review]
Single select

Cleaned up a bit.
Attachment #569748 - Attachment is obsolete: true
Attachment #569779 - Flags: review?(mark.finkle)
(Assignee)

Comment 4

6 years ago
Created attachment 569781 [details] [diff] [review]
Multiple Select

Support for multiple select
Attachment #569781 - Flags: review?(mark.finkle)
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 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.
(Assignee)

Comment 7

6 years ago
> >+  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...
(Assignee)

Comment 8

6 years ago
Created attachment 569840 [details] [diff] [review]
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/
(Assignee)

Updated

6 years ago
Attachment #569840 - Flags: review?(mark.finkle)
(Assignee)

Comment 9

6 years ago
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).
(Assignee)

Comment 10

6 years ago
Created attachment 569849 [details] [diff] [review]
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
Attachment #569840 - Attachment is obsolete: true
Attachment #569840 - Flags: review?(mark.finkle)
(Assignee)

Comment 11

6 years ago
Created attachment 570132 [details] [diff] [review]
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.
Attachment #569779 - Attachment is obsolete: true
Attachment #569779 - Flags: review?(mark.finkle)
Attachment #570132 - Flags: review?(mark.finkle)
(Assignee)

Comment 12

6 years ago
Created attachment 570134 [details] [diff] [review]
Multiple Select v2

This has almost no changes except moving "Done" to browser.properties.
Attachment #569781 - Attachment is obsolete: true
Attachment #569781 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Attachment #570134 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Attachment #569849 - Flags: review?(mark.finkle)
Attachment #570132 - Flags: review?(mark.finkle) → review+
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
Attachment #570134 - Flags: review?(mark.finkle) → review+
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.
Attachment #569849 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/projects/birch/rev/ec7d971fc82c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

6 years ago
Created attachment 570343 [details] [diff] [review]
Missing file

Forgot to hg add this file...
Attachment #570343 - Flags: review?(mark.finkle)
Attachment #570343 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 17

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

Updated

6 years ago
Depends on: 698114

Updated

6 years ago
Depends on: 698452
(Reporter)

Updated

6 years ago
tracking-fennec: --- → 11+
(Reporter)

Updated

6 years ago
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.