Closed Bug 731654 Opened 12 years ago Closed 12 years ago

Style HTML5 form validation popup

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox13 fixed, firefox14 fixed, blocking-fennec1.0 beta+)

VERIFIED FIXED
Firefox 14
Tracking Status
firefox13 --- fixed
firefox14 --- fixed
blocking-fennec1.0 --- beta+

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

(Keywords: uiwanted)

Attachments

(5 files, 7 obsolete files)

I decided to break this off from bug 704879.
blocking-fennec1.0: --- → ?
This is blocking beta as bug 704879 is blocking beta.
blocking-fennec1.0: ? → beta+
I'll get crackin' on this once Ian posts some designs/assets :)
Assignee: nobody → margaret.leibovic
Attached image 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)
Refactoring variable names so that they'll make sense for both the suggestions list case, as well as the form validation message case.
Attachment #603926 - Flags: review?(sriram)
Some more refactoring that will make it easier to support two popup styles.
Attachment #603927 - Flags: review?(sriram)
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.
Attachment #603931 - Flags: review?(sriram)
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.
Attachment #603933 - Flags: feedback?(sriram)
Attached file popup assets (obsolete) —
Hi Margaret, here are the assets for the popup!
Attached file popup assets
Oops, I messed up the previous 9patch, please use these instead :)
Attachment #603944 - Attachment is obsolete: true
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?
Attachment #603926 - Flags: review?(sriram) → review+
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.
Attachment #603927 - Flags: review?(sriram) → review-
Comment on attachment 603931 [details] [diff] [review]
(3/4) Get rid of mLayout/mHeight/mWidth

This looks good to me.
Attachment #603931 - Flags: review?(sriram) → review+
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.
Attachment #603933 - Flags: feedback?(sriram) → feedback-
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 )
(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).
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.
blocking-fennec1.0: beta+ → +
(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).
This is rebased to work without patch (2/4) - we can just forget about that patch.
Attachment #603927 - Attachment is obsolete: true
Attachment #603931 - Attachment is obsolete: true
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).
Attachment #603933 - Attachment is obsolete: true
Attachment #604166 - Flags: review?(sriram)
Attached image screenshot on gingerbread (obsolete) —
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).
Attached image screenshot on ICS (obsolete) —
Same thing on ICS.
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.
Attachment #604166 - Flags: review?(sriram) → review+
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.
Attachment #604166 - Attachment is obsolete: true
Attachment #604166 - Flags: review+
blocking-fennec1.0: + → beta+
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/
Attachment #604169 - Attachment is obsolete: true
Attachment #604170 - Attachment is obsolete: true
Attachment #607788 - Flags: review?(sriram)
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!
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.
Depends on: 737907
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.
Attachment #607788 - Flags: review?(sriram) → review+
Depends on: 737922
Comment on attachment 603926 [details] [diff] [review]
(1/3) Refactor listFoo -> popupFoo

[Approval Request Comment]
Mobile only and blocking-fennec1.0.
Attachment #603926 - Attachment description: (1/4) Refactor listFoo -> popupFoo → (1/3) Refactor listFoo -> popupFoo
Attachment #603926 - Flags: approval-mozilla-aurora?
Comment on attachment 604164 [details] [diff] [review]
(2/3) Get rid of mLayout/mHeight/mWidth

[Approval Request Comment]
Mobile only and blocking-fennec1.0.
Attachment #604164 - Attachment description: (3/4) Get rid of mLayout/mHeight/mWidth → (2/3) Get rid of mLayout/mHeight/mWidth
Attachment #604164 - Flags: approval-mozilla-aurora?
Comment on attachment 607788 [details] [diff] [review]
(3/3) Style form validation popup (v2)

[Approval Request Comment]
Mobile only and blocking-fennec1.0.
Attachment #607788 - Attachment description: (4/4) Style form validation popup (v2) → (3/3) Style form validation popup (v2)
Attachment #607788 - Flags: approval-mozilla-aurora?
Comment on attachment 603926 [details] [diff] [review]
(1/3) Refactor listFoo -> popupFoo

[Triage Comment]
Mobile only - approved for Aurora 13.
Attachment #603926 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #604164 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #607788 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified/fixed on:
Nightly/15.0a1 2012-04-25/ Aurora/13.0a2 2012-04-24
Device: HTC Desire (Android 2.2.2)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: