Last Comment Bug 731654 - Style HTML5 form validation popup
: Style HTML5 form validation popup
Status: VERIFIED FIXED
: uiwanted
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 14
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on: 737907 737922
Blocks: 704879
  Show dependency treegraph
 
Reported: 2012-02-29 09:18 PST by :Margaret Leibovic
Modified: 2012-04-26 04:37 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
beta+


Attachments
Mockup (26.83 KB, image/png)
2012-03-07 06:42 PST, Ian Barlow (:ibarlow)
no flags Details
(1/3) Refactor listFoo -> popupFoo (4.05 KB, patch)
2012-03-07 17:45 PST, :Margaret Leibovic
sriram.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
(2/4) Factor variable initialization out of positionAndShowPopup (4.30 KB, patch)
2012-03-07 17:46 PST, :Margaret Leibovic
sriram.mozilla: review-
Details | Diff | Review
(3/4) Get rid of mLayout/mHeight/mWidth (4.47 KB, patch)
2012-03-07 17:50 PST, :Margaret Leibovic
sriram.mozilla: review+
Details | Diff | Review
(4/4) WIP - Style form validation popup (17.08 KB, patch)
2012-03-07 17:52 PST, :Margaret Leibovic
sriram.mozilla: feedback-
Details | Diff | Review
popup assets (2.75 KB, application/zip)
2012-03-07 18:27 PST, Ian Barlow (:ibarlow)
no flags Details
popup assets (2.76 KB, application/zip)
2012-03-07 18:37 PST, Ian Barlow (:ibarlow)
no flags Details
(2/3) Get rid of mLayout/mHeight/mWidth (4.57 KB, patch)
2012-03-08 12:53 PST, :Margaret Leibovic
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
(4/4) Style form validation popup (19.38 KB, patch)
2012-03-08 12:58 PST, :Margaret Leibovic
no flags Details | Diff | Review
screenshot on gingerbread (100.33 KB, image/png)
2012-03-08 13:02 PST, :Margaret Leibovic
no flags Details
screenshot on ICS (95.37 KB, image/png)
2012-03-08 13:02 PST, :Margaret Leibovic
no flags Details
(3/3) Style form validation popup (v2) (22.24 KB, patch)
2012-03-20 17:32 PDT, :Margaret Leibovic
sriram.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description :Margaret Leibovic 2012-02-29 09:18:08 PST
I decided to break this off from bug 704879.
Comment 1 Kevin Brosnan [:kbrosnan] 2012-03-01 13:54:41 PST
This is blocking beta as bug 704879 is blocking beta.
Comment 2 :Margaret Leibovic 2012-03-06 08:12:11 PST
I'll get crackin' on this once Ian posts some designs/assets :)
Comment 3 Ian Barlow (:ibarlow) 2012-03-07 06:42:31 PST
Created attachment 603696 [details]
Mockup

Hi Margaret, let's try something like this, a grey box with a pointer on top. 

Ping me on IRC today and let me know how you'd like the assets set up (or of you'd just prefer a PSD)
Comment 4 :Margaret Leibovic 2012-03-07 17:45:18 PST
Created attachment 603926 [details] [diff] [review]
(1/3) Refactor listFoo -> popupFoo

Refactoring variable names so that they'll make sense for both the suggestions list case, as well as the form validation message case.
Comment 5 :Margaret Leibovic 2012-03-07 17:46:45 PST
Created attachment 603927 [details] [diff] [review]
(2/4) Factor variable initialization out of positionAndShowPopup

Some more refactoring that will make it easier to support two popup styles.
Comment 6 :Margaret Leibovic 2012-03-07 17:50:37 PST
Created attachment 603931 [details] [diff] [review]
(3/4) Get rid of mLayout/mHeight/mWidth

mHeight wasn't actually being used at all (popupHeight is always getting re-assinged), mWidth just stayed -1 (RelativeLayout.LayoutParams.FILL_PARENT) forever because we never reset it, and mLayout was only being used right where it was set, so it doesn't need to be a member variable.

I also moved the popupTop declaration closer to where it's being used.
Comment 7 :Margaret Leibovic 2012-03-07 17:52:14 PST
Created attachment 603933 [details] [diff] [review]
(4/4) WIP - Style form validation popup

I still need the real assets from ibarlow, but this is a WIP with the doorhanger assets. It seemed to work well on ICS, but I just tested on Gingerbread, and it looks like there are some padding issues.
Comment 8 Ian Barlow (:ibarlow) 2012-03-07 18:27:22 PST
Created attachment 603944 [details]
popup assets

Hi Margaret, here are the assets for the popup!
Comment 9 Ian Barlow (:ibarlow) 2012-03-07 18:37:29 PST
Created attachment 603947 [details]
popup assets

Oops, I messed up the previous 9patch, please use these instead :)
Comment 10 Sriram Ramasubramanian [:sriram] 2012-03-07 21:41:52 PST
Comment on attachment 603926 [details] [diff] [review]
(1/3) Refactor listFoo -> popupFoo

I am fine with renaming variables. But I am not sure if we need a TextView as shown in patch 4. Are AutocompletePopup and FormAssist Popup going to use different styles?
Comment 11 Sriram Ramasubramanian [:sriram] 2012-03-07 21:49:02 PST
Comment on attachment 603927 [details] [diff] [review]
(2/4) Factor variable initialization out of positionAndShowPopup

I don't want to have a separate function for initDisplayData(). The use of static variable solves the purpose. 

if (sMinWidth == 0) {
// do the initialization
}

feels better to me. Also, since mHeight is going away in subsequent patches, I would be happy to used static variable than a function and a private variable.
Comment 12 Sriram Ramasubramanian [:sriram] 2012-03-07 21:51:37 PST
Comment on attachment 603931 [details] [diff] [review]
(3/4) Get rid of mLayout/mHeight/mWidth

This looks good to me.
Comment 13 Sriram Ramasubramanian [:sriram] 2012-03-07 21:59:48 PST
Comment on attachment 603933 [details] [diff] [review]
(4/4) WIP - Style form validation popup

I would like to have FormAssistPopup as a container which holds the Positioning logic.

So the gecko_app.xml will have

<org.moz...FormAssistPopup android:id="@+id/autocomplete" />
<org.moz...FormAssistPopup android:id="@+id/form_assist" />

Please inflate the contents based on need. Autocomplete Popup and HTML5 Form assist is not needed for every website visited by the users. We can inflate it only when the user needs it. That too on startup path, I wouldn't recommend doing it.

+    private static final int SUGGESTIONS_MIN_WIDTH_IN_DPI = 200; 

This is still popup's width and not suggestions. Please have the old name.

String value = (String) textView.getTag();

Why don't we hold the adapter in a private variable and just get values from it? Also,

String value = (String) ((TextView) view).getTag();

is better.
Comment 14 Sriram Ramasubramanian [:sriram] 2012-03-07 22:04:24 PST
Also, see if we can add the "arrow + background" as a single 9-png image. Thereby we wouldn't need a separate "arrow ImageView". I think it should be do-able.

DoorHanger cannot do it because, the arrow is placed somewhere to the left and things get crazy.

(There too, things might get crazy when we switch to tablets.. but I guess the showAsDropDown() will save us :D )
Comment 15 :Margaret Leibovic 2012-03-08 10:32:43 PST
(In reply to Sriram Ramasubramanian [:sriram] from comment #11)
> Comment on attachment 603927 [details] [diff] [review]
> (2/4) Factor variable initialization out of positionAndShowPopup
> 
> I don't want to have a separate function for initDisplayData(). The use of
> static variable solves the purpose. 
> 
> if (sMinWidth == 0) {
> // do the initialization
> }
> 
> feels better to me. Also, since mHeight is going away in subsequent patches,
> I would be happy to used static variable than a function and a private
> variable.

I'm fine to leave out this change, but my motivation for doing this was 1) to get some stuff out of positionAndShowPopup, since it's a pretty big method, and 2) be more semantically correct about deciding when to initialize the static variables (when we're checking |sMinWidth == 0|, what we're really checking is if we initialized all these static variables).
Comment 16 Sriram Ramasubramanian [:sriram] 2012-03-08 10:35:19 PST
I agree that part, however, we are doing an unnecessary function call, an extra variable to see if everything is initialized and a condition to check it every call. | sMinWidth == 0 | doesn't involve a new function call and an extra variable. The condition check alone exists.
Comment 17 :Margaret Leibovic 2012-03-08 11:50:34 PST
(In reply to Sriram Ramasubramanian [:sriram] from comment #13)

> +    private static final int SUGGESTIONS_MIN_WIDTH_IN_DPI = 200; 
> 
> This is still popup's width and not suggestions. Please have the old name.

In this patch it's actually only being used for the autocomplete suggestions case. I'm not sure if we want a min width for the validation message case, since the messages are always relatively long (that's also an easy way to make sure the message is centered in the popup).

> String value = (String) textView.getTag();
> 
> Why don't we hold the adapter in a private variable and just get values from
> it? Also,
> 
> String value = (String) ((TextView) view).getTag();
> 
> is better.

This isn't part of this patch, so I don't think we need to change it here. Also, this is funny - see bug 704879 comment 12:

>>          setOnItemClickListener(new OnItemClickListener() {
>>              public void onItemClick(AdapterView<?> parentView, View view, int position, long id) {
>> +                if (mTypeShowing == TYPE_AUTOCOMPLETE) {
>> +                    String value = ((TextView) view).getText().toString();
>
>I generally try to avoid those in-place castings for the sake of readability. Cast >to a temp variable then use it here.

(In reply to Sriram Ramasubramanian [:sriram] from comment #14)
> Also, see if we can add the "arrow + background" as a single 9-png image.
> Thereby we wouldn't need a separate "arrow ImageView". I think it should be
> do-able.
> 
> DoorHanger cannot do it because, the arrow is placed somewhere to the left
> and things get crazy.
> 
> (There too, things might get crazy when we switch to tablets.. but I guess
> the showAsDropDown() will save us :D )

This doesn't seem like a blocking issue, so it would probably be best done in another bug (when we have more time to play around with getting it to work).
Comment 18 :Margaret Leibovic 2012-03-08 12:53:52 PST
Created attachment 604164 [details] [diff] [review]
(2/3) Get rid of mLayout/mHeight/mWidth

This is rebased to work without patch (2/4) - we can just forget about that patch.
Comment 19 :Margaret Leibovic 2012-03-08 12:58:53 PST
Created attachment 604166 [details] [diff] [review]
(4/4) Style form validation popup

This patch lazily inflates the layouts. I still kept one FormAssistPopup, since GeckoApp keeps track of mFormAssistPopup, and we reference that in a bunch of places to hide the popup in different situations. So it's the same idea as the last patch - there are two different kinds of content that can appear inside FormAssistPopup, we just show one at a time (and only inflate them when we need them).
Comment 20 :Margaret Leibovic 2012-03-08 13:02:09 PST
Created attachment 604169 [details]
screenshot on gingerbread

I found that the border where the arrow joins the background looks a little wrong, like they're overlapping. However, when I tried increasing the space between them, a definite gap appeared. Maybe this has to do with using dip for the margin/height values? If we can't come up with an immediate solution, this feels like something that can be done in a follow up (maybe we can use one arrow/background image as Sriram suggested).
Comment 21 :Margaret Leibovic 2012-03-08 13:02:35 PST
Created attachment 604170 [details]
screenshot on ICS

Same thing on ICS.
Comment 22 Sriram Ramasubramanian [:sriram] 2012-03-08 14:55:06 PST
Comment on attachment 604166 [details] [diff] [review]
(4/4) Style form validation popup

+        // Hide autocomplete list if it exists, and show validation message
+        if (mAutoCompleteList != null)
+            mAutoCompleteList.setVisibility(GONE);
+        mValidationMessage.setVisibility(VISIBLE);

Could you move this to a function?
Probably positionAndShowPopup() can use the extra argument and do this there.

+  res/drawable/validation_arrow.png \
+  res/drawable/validation_bg.9.png \

You might need separate resources for hdpi and xhdpi too. (DoorHanger has it).

+    <ImageView android:layout_width="wrap_content"

Use actual width in "dp". Can't we reuse doorhanger arrow and assets for this?

Other than that, things look fine to me.
Comment 23 :Margaret Leibovic 2012-03-20 11:58:12 PDT
Comment on attachment 604166 [details] [diff] [review]
(4/4) Style form validation popup

Clearing r+, since I found problems with the positioning logic, so I'm going to work on a new version.
Comment 24 :Margaret Leibovic 2012-03-20 17:32:59 PDT
Created attachment 607788 [details] [diff] [review]
(3/3) Style form validation popup (v2)

This patch addresses our concerns about messages that are too long by making the popup grow as wide as the screen if necessary, and then if the message is still too long, the text will marquee. Since we will never wrap the text, this allows us to set a fixed height for the validation message, and we can use that in positionAndShowPopup.

I made added some comments to positionAndShowPopup to make it clearer what's going on (mostly for my own benefit as I was working this out :). I found that we actually only need to set the width of the popup for the autocomplete suggestions case, since that's when we want the message to be the same width as the input.

One case that this doesn't handle perfectly is the case where we end up needing to position the popup above the input box, since I didn't add any code to flip the arrow to the bottom of the message instead of the top. However, this seems like an edge case that we could handle in a follow-up, especially since it might be tricky to do that.

I put screenshots of long/short messages on ICS/gingerbread here:
http://people.mozilla.com/~mleibovic/form-validation/
Comment 25 :Margaret Leibovic 2012-03-20 17:37:26 PDT
Ian, I'm still seeing that same weirdness where the arrow joins the background. I think it might have to do with the height of the arrow. This is something we can work out in a follow-up bug, but let's find each other on IRC tomorrow to work it out!
Comment 26 Ian Barlow (:ibarlow) 2012-03-21 06:39:55 PDT
Margaret, sounds good. With the exception of the arrow / background wierdness, the rest of the popup looks great. I'm good to move the last bit to a follow up bug.
Comment 27 Sriram Ramasubramanian [:sriram] 2012-03-21 10:18:10 PDT
Comment on attachment 607788 [details] [diff] [review]
(3/3) Style form validation popup (v2)

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

This looks good to me. If you want to invert the arrow, please file a followup.
Comment 30 :Margaret Leibovic 2012-03-22 10:03:05 PDT
Comment on attachment 603926 [details] [diff] [review]
(1/3) Refactor listFoo -> popupFoo

[Approval Request Comment]
Mobile only and blocking-fennec1.0.
Comment 31 :Margaret Leibovic 2012-03-22 10:03:34 PDT
Comment on attachment 604164 [details] [diff] [review]
(2/3) Get rid of mLayout/mHeight/mWidth

[Approval Request Comment]
Mobile only and blocking-fennec1.0.
Comment 32 :Margaret Leibovic 2012-03-22 10:03:55 PDT
Comment on attachment 607788 [details] [diff] [review]
(3/3) Style form validation popup (v2)

[Approval Request Comment]
Mobile only and blocking-fennec1.0.
Comment 33 Alex Keybl [:akeybl] 2012-03-22 13:43:54 PDT
Comment on attachment 603926 [details] [diff] [review]
(1/3) Refactor listFoo -> popupFoo

[Triage Comment]
Mobile only - approved for Aurora 13.
Comment 35 Adrian Tamas (:AdrianT) 2012-04-26 04:37:00 PDT
Verified/fixed on:
Nightly/15.0a1 2012-04-25/ Aurora/13.0a2 2012-04-24
Device: HTC Desire (Android 2.2.2)

Note You need to log in before you can comment on or make changes to this bug.