Style HTML5 form validation popup

VERIFIED FIXED in Firefox 13

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

({uiwanted})

Trunk
Firefox 14
ARM
Android
uiwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(5 attachments, 7 obsolete attachments)

(Assignee)

Description

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

Comment 2

6 years ago
I'll get crackin' on this once Ian posts some designs/assets :)
Assignee: nobody → margaret.leibovic
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)
(Assignee)

Comment 4

6 years ago
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.
Attachment #603926 - Flags: review?(sriram)
(Assignee)

Comment 5

6 years ago
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.
Attachment #603927 - Flags: review?(sriram)
(Assignee)

Comment 6

6 years ago
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.
Attachment #603931 - Flags: review?(sriram)
(Assignee)

Comment 7

6 years ago
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.
Attachment #603933 - Flags: feedback?(sriram)
Created attachment 603944 [details]
popup assets

Hi Margaret, here are the assets for the popup!
Created attachment 603947 [details]
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 )
(Assignee)

Comment 15

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

Comment 17

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

Comment 18

6 years ago
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.
Attachment #603927 - Attachment is obsolete: true
Attachment #603931 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
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).
Attachment #603933 - Attachment is obsolete: true
Attachment #604166 - Flags: review?(sriram)
(Assignee)

Comment 20

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

Comment 21

6 years ago
Created attachment 604170 [details]
screenshot on ICS

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+
(Assignee)

Comment 23

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

Comment 24

6 years ago
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/
Attachment #604169 - Attachment is obsolete: true
Attachment #604170 - Attachment is obsolete: true
Attachment #607788 - Flags: review?(sriram)
(Assignee)

Comment 25

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

Updated

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

Updated

6 years ago
Depends on: 737922
(Assignee)

Comment 28

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b699a2a064
https://hg.mozilla.org/integration/mozilla-inbound/rev/882854cf6462
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b54e87e03e
https://hg.mozilla.org/mozilla-central/rev/75b699a2a064
https://hg.mozilla.org/mozilla-central/rev/882854cf6462
https://hg.mozilla.org/mozilla-central/rev/67b54e87e03e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
(Assignee)

Comment 30

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

Comment 31

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

Comment 32

6 years ago
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+

Updated

6 years ago
Attachment #604164 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #607788 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad95d6c0817e
https://hg.mozilla.org/releases/mozilla-aurora/rev/c815f9d5654b
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad95d6c0817e
status-firefox13: --- → fixed
status-firefox14: --- → fixed
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
You need to log in before you can comment on or make changes to this bug.