Closed Bug 1007436 Opened 5 years ago Closed 5 years ago

Revamp visual design of about:feedback

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 35
Tracking Status
firefox35 --- verified
firefox36 --- verified
firefox37 --- verified

People

(Reporter: liuche, Assigned: vivek, Mentored)

References

Details

(Whiteboard: [lang=html][lang=js][lang=css])

Attachments

(13 files, 19 obsolete files)

236.22 KB, image/png
Details
175.73 KB, image/png
Details
188.93 KB, image/png
Details
231.58 KB, image/png
Details
48.84 KB, application/zip
Details
133.79 KB, image/png
antlam
: feedback-
Details
316.53 KB, image/png
antlam
: feedback-
Details
148.78 KB, image/png
Details
87.71 KB, image/png
Details
52.21 KB, image/png
Details
80.91 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
1.12 MB, application/zip
antlam
: review+
Details
80.92 KB, patch
Margaret
: review+
antlam
: feedback+
Details | Diff | Splinter Review
Some of the negative feedback is for features that exist in FF, but the user doesn't know about; there also tend to be several common recurring problems.

We could link to the FAQ somewhere on the negative feedback form.

Maybe adding a string along the lines of, "You can also check out our FAQs for common problems."
I like this idea! We already have a link to SUMO at the bottom of the negative feedback form, but maybe we should make that more prominent, or re-word it to direct to the FAQ page instead of the SUMO landing page.

For anyone working on this, you'll want to take a look at the code here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutFeedback.xhtml#67
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutFeedback.js#39

I also wonder if we can track how often users are actually clicking on this link. The URL we use is formatted for the app version and locale, so the server must be receiving some information about this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#466
Whiteboard: [mentor=margaret][lang=html][lang=js[
Whiteboard: [mentor=margaret][lang=html][lang=js[ → [mentor=margaret][lang=html][lang=js]
Hi Margaret.

Could you please guide I can proceed to this bug?
Flags: needinfo?(margaret.leibovic)
(In reply to Biraj Karmakar [:biraj] from comment #2)
> Hi Margaret.
> 
> Could you please guide I can proceed to this bug?

Sure! First of all, do you have a build environment set up? If not, you'll want to follow the directions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

After that, you should play around with the files that I pointed to in my last comment.

Unfortunately, I don't have a clear idea of exactly what change we should make here. At the very least, I think we should consider making the "You can also visit Firefox Support for more information" text a bit larger, but maybe we should re-word it to make it sound more helpful. Or maybe we should make it part of the text at the top of the page above the text area.

ibarlow, what do you think?
Flags: needinfo?(margaret.leibovic) → needinfo?(ibarlow)
Attached image Screenshots
Summoning Anthony!

Anthony, we want to increase the prominence of our Support page on about:feedback. The idea is that if people are on this page, it's because they have problems, and if they have problems, maybe we can help them with something they just didn't know about that's already on our Support site. 

Can you think about what this might look like?
Flags: needinfo?(ibarlow) → needinfo?(alam)
Just to clarify, this is a simple form submission page that responds, and has the above content?

I will give this a look soon.
Flags: needinfo?(alam)
Yep. And the goal here is to add a little more emphasis to the "Visit our support page" action
To test this out as it currently exists, you can visit about:feedback in Fennec. Right now we open a new tab with this page after the user has launched the app a certain number of times.
Hi everyone. I'm new to mozilla community. I want to work on this bug. Please assign this bug to me.
I have set up the build environment as suggested in comment #3.

I'm more comfortable with using Git than Mercurial. I have forked and cloned this repo https://github.com/mozilla/gecko-dev.

this is the one right if i want to work through git ?

Please give me a little more instructions on going about adding the features discussed above.

Thanks,
Sunil
I have some problems in building. When i execute
./mach build
I'm getting the below error.

 0:00.22 /usr/bin/make -f client.mk -s
 0:00.46 Traceback (most recent call last):
 0:00.46   File "/home/sunil/repos/git/gecko-dev/config/pythonpath.py", line 56, in <module>
 0:00.46     main(sys.argv[1:])
 0:00.46   File "/home/sunil/repos/git/gecko-dev/config/pythonpath.py", line 48, in main
 0:00.46     execfile(script, frozenglobals)
 0:00.46 NameError: name 'execfile' is not defined
 0:00.47 client.mk:345: recipe for target 'obj-x86_64-unknown-linux-gnu/CLOBBER' failed
 0:00.47 make: *** [obj-x86_64-unknown-linux-gnu/CLOBBER] Error 1
 0:00.49 0 compiler warnings present


Please help. This is my first time building the product.
Attached image preview_feedbackpanel1.png (obsolete) —
Attaching designs for this page. 

After reading a lot of feedback off input.mozilla.org I realized that we are doing a 1 character requirement in order for the user to submit the form. Might I suggest either none, or more than 1? 1 seems kind of arbitrary. From this bit of research I also condensed the size of the input box. I think it could either expand accordingly or just scroll within (we'll have to play with that). 

The "Send Feedback" button also has an inactive state (depicted left) and an active state (blue). 

I also want to see if we can get the "Include last visited site" to be a check box rather than an input field since I assume this link is automatically generated anyways and with long strings, the user would have to do take a few more actions to delete it.

Finally, I went with a warmer tone background than our current blue to give a warmer feeling and added a whimsical heart at the end to help provide a better wrap up for the user than just plain text (the copy is already whimsy to match! :)
Overall the new mockups look good. Ian can bless the general style changes. I don't think we _need_ to have the Feedback Form look like the other "about" pages (thinking of add-ons and downloads mainly) but we should conciously make the choice to split them visually. Maybe we will consider converting about:addons and about:downloads and about:apps to look more like this mockup.

I wonder if we originally made the "last visited site" an input so the user could see what we were sending. Just a thought.

Lastly, and perhaps my primary feedback: Does "Nightly Support" give enough context to indicate clicking the link (button) is a way to get help and answers? I think using words like "Help" or "Frequently asked questions" would give a better context. I don't like "FAQ" since I don't believe all people really know what that means.
Attached image feedback
This is looking great Anthony, I love the visual changes. 

I have some comments attached here, mostly around visual hierarchy and grouping. The key thing is, the link to support and the flow through the feedback inputs are starting to feel like they are part of the same thing now, and they should probably feel more like two distinct thoughts.
(In reply to Sunil Kumar from comment #9)
> I have some problems in building. When i execute
> ./mach build
> I'm getting the below error.
> 
>  0:00.22 /usr/bin/make -f client.mk -s
>  0:00.46 Traceback (most recent call last):
>  0:00.46   File "/home/sunil/repos/git/gecko-dev/config/pythonpath.py", line
> 56, in <module>
>  0:00.46     main(sys.argv[1:])
>  0:00.46   File "/home/sunil/repos/git/gecko-dev/config/pythonpath.py", line
> 48, in main
>  0:00.46     execfile(script, frozenglobals)
>  0:00.46 NameError: name 'execfile' is not defined
>  0:00.47 client.mk:345: recipe for target
> 'obj-x86_64-unknown-linux-gnu/CLOBBER' failed
>  0:00.47 make: *** [obj-x86_64-unknown-linux-gnu/CLOBBER] Error 1
>  0:00.49 0 compiler warnings present
> 
> 
> Please help. This is my first time building the product.

I'm sorry nobody jumped in to help you out here! Were you able to get your build working? If not, you should join #mobile on irc.mozilla.org for real-time help.
Flags: needinfo?(sunilkumar.c682)
Attached image preview_feedbackpanel2.png (obsolete) —
Attaching new mock ups for this page. 

I have revised with the previous feedback in mind and also come up with 3 different copy strings that we could try out. The over aesthetic has taken from the Sync Snippet that we currently use on about:home. This would be anchored to the bottom of the page but might create some problems when the keyboard is up (we'd have to make sure it doesn't float above the keyboard I think). 

But hopefully this creates the separation that you were looking for? Thoughts?
Attachment #8430402 - Attachment is obsolete: true
Flags: needinfo?(ibarlow)
This looks great Anthony. I would just tweak the string a little and say this is good to go. 

"Visit our _Help section_ to get answers on common issues."
Flags: needinfo?(ibarlow)
Alright, I think we have a winner?
Attachment #8432841 - Attachment is obsolete: true
Looking very nice. The current mockups are Fx33 at the earliest. Do we have any thoughts or desire for a Fx32 quick fix that could land this week?
(In reply to Mark Finkle (:mfinkle) from comment #17)
> Looking very nice. The current mockups are Fx33 at the earliest. Do we have
> any thoughts or desire for a Fx32 quick fix that could land this week?

If we do, I can jump in to write a patch for this bug. However, since this has been a long-standing issue, I think it would be fine if we end up doing something in 33 instead.

These mock-ups do look really nice, perhaps we can finish bug 878173 at the same time, and then Fx33 can have an overall improved about:feedback.
Attached image preview_feedbackpanel5.png (obsolete) —
Couldn't help but think that the copy on these pages were getting a bit overwhelming and I wanted to catch this before we closed the bug. 

In the new copy, I tried to keep the same whimsy but in a more brief manner. 

Thoughts?
Thanks Anthony. I see what you did there, and it's nice and short -- but I think you cut too much... :) I would roll back to the previous copy

Using things like "please" and "for your privacy" were pretty deliberate decisions to make this feel like it was written by people, and I think we've lost a bit of that tone in this latest version.
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][lang=html][lang=js] → [lang=html][lang=js]
Comment on attachment 8435295 [details]
preview_feedbackpanel5.png

Obsoleting based on Ian's feedback
Attachment #8435295 - Attachment is obsolete: true
Hi Margaret,

I would like to take a shot at this. However, I would need your guidance in solving this.
(In reply to vivek from comment #22)
> Hi Margaret,
> 
> I would like to take a shot at this. However, I would need your guidance in
> solving this.

Hi vivek! Sorry this comment slipped through the cracks...

I'm actually not sure what our plan is for this bug right now. I believe the scope expanded to revamp the style of the entire about:feedback page.

I was already working on something similar in bug 878173, but perhaps we should morph this bug to just be about updating the style of our current about:feedback, which would include adding a more prominent help link.

antlam, are the mock-ups in this bug still valid, or should we wait for updated designs?

vivek, if you want to start looking through the code, the relevant files are here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutFeedback.xhtml
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutFeedback.js
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutFeedback.css

This page is made with 100% web technology, so you can use the remote developer tools to inspect in while you work on it:
https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Firefox_for_Android
Flags: needinfo?(sunilkumar.c682) → needinfo?(alam)
Summary: Add help FAQ link to negative feedback form → Revamp visual design of about:feedback
Blocks: 878173
Whiteboard: [lang=html][lang=js] → [lang=html][lang=js][lang=css]
Nit for redesign (maybe?): currently you see a success message if the device is offline (if you attempt to submit). I was playing around in Airplane mode and noticed this yesterday.
Attached patch 1007436.patch (obsolete) — Splinter Review
Hi Margaret,
Please find a patch based on the attachment 8433376 [details].
Attachment #8469666 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8469666 [details] [diff] [review]
1007436.patch

>diff --git a/mobile/android/locales/en-US/chrome/aboutFeedback.dtd b/mobile/android/locales/en-US/chrome/aboutFeedback.dtd

>-<!ENTITY support.pre2              "You can also visit ">
>-<!ENTITY support.link              "&brandShortName; Support">
>-<!ENTITY support.post2             " for more information.">
>+<!ENTITY support.pre2              "Visit our ">
>+<!ENTITY support.link              "Help Section">
>+<!ENTITY support.post2             " to get answers on common issues.">

>-<!ENTITY sad.lastSite              "Last visited site (optional)">
>+<!ENTITY sad.lastSite              "Include last visited site">

Just a drive-by. Any string entities (like support.pre2) that change the string value must also change the entity too. So support.pre2 becomes support.pre3 and so on. You only need to do this for string changes. Also, if it makes sense to use a completely different entity name, do it. Like maybe sad.lastSite could become sad.includeLastSite if it's really different enough from the original purpose. Otherwise, just bump it to sad.lastSite2
Oops. I didn't tell you "why" we do this. Changing the entity is a trigger for the tools the L10N people use to translate the strings. It tells them that a string has changed an needs to be re-translated.
(In reply to :Margaret Leibovic from comment #23)
> (In reply to vivek from comment #22)
> > Hi Margaret,
> > 
> > I would like to take a shot at this. However, I would need your guidance in
> > solving this.
> 
> Hi vivek! Sorry this comment slipped through the cracks...
> 
> I'm actually not sure what our plan is for this bug right now. I believe the
> scope expanded to revamp the style of the entire about:feedback page.
> 
> I was already working on something similar in bug 878173, but perhaps we
> should morph this bug to just be about updating the style of our current
> about:feedback, which would include adding a more prominent help link.
> 
> antlam, are the mock-ups in this bug still valid, or should we wait for
> updated designs?

I think they're still good :) We just need to address issues in practice after we see it living.
Flags: needinfo?(alam)
Comment on attachment 8469666 [details] [diff] [review]
1007436.patch

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

Thanks for getting started on this bug! Could you attach screenshots or an APK for antlam to look at? In general, when working on any visual changes it's always really helpful to share work with our UX team as you go.

This looks like it's going in the right direction, but there's still a lot of work to do to get the styles correct. You should also ask antlam for any icons you need as you're working on this.

::: mobile/android/chrome/content/aboutFeedback.xhtml
@@ +62,5 @@
>        <textarea class="description" placeholder="&sad.placeholder;" rows="8" required="true"/>
> +      <div class="message">
> +        <span>&sad.lastSite;</span>
> +        <input id="last-checkbox" type="checkbox"/>
> +        <input id="last-url" type="url" placeholder="&sad.urlPlaceholder;" readonly="readonly"/>

In the mock-up, it looks like this last visited URL isn't editable, so this shouldn't be an input anymore.

We should also check with antlam about what we should do here if we don't have a last visited URL. Previously we would just show the "http://" placeholder, but perhaps we should disable the checkbox if there isn't a URL to share.

::: mobile/android/locales/en-US/chrome/aboutFeedback.dtd
@@ +16,5 @@
>  
>  <!-- LOCALIZATION NOTE (support.pre): Include a trailing space as needed. -->
>  <!-- LOCALIZATION NOTE (support.link): Avoid leading/trailing spaces, this text is a link. -->
>  <!-- LOCALIZATION NOTE (support.post): Include a starting space as needed. -->
> +<!ENTITY support.pre2              "Visit our ">

I see mfinkle already pointed out that you need to rev these entity names :)

::: mobile/android/themes/core/aboutFeedback.css
@@ -94,5 @@
>  
>  @media not screen and (max-height: 400px) {
> -  body {
> -    padding-top: 40px;
> -  }

Why did you get rid of this padding?

@@ +137,5 @@
>    width: -moz-calc(100% - 10px);
>  }
>  
> +#last-url {
> +  font-family: "Clear Sans",sans-serif;

It looks like the font should be the same for the whole page. Is this needed? I would think the body font-family style would take care of it for us.

@@ +149,5 @@
> +#last-checkbox {
> +  position: relative;
> +  float: right;
> +  margin: 0px;
> +  background-image: url("chrome://browser/skin/images/checkbox_unchecked.png");

We should get icons from antlam for this.

@@ +157,5 @@
> +  background-image: url("chrome://browser/skin/images/checkbox_checked.png");
> +}
> +
> +#last-checkbox:not(:checked) ~ #last-url {
> +  display: none;

With this style, the contents below the last URL get pushed up and down when you check/uncheck the box. It looks like that's in the design, but it feels a bit jarring. Maybe we should see if we can just make this URL invisible if the box isn't checked.
Attachment #8469666 - Flags: feedback?(margaret.leibovic) → feedback+
Assignee: nobody → vivekb.balakrishnan
Attached patch revamp_feedback.patch (obsolete) — Splinter Review
A new patch which incorporates the reviews from mfinkle & margaret .

::: mobile/android/themes/core/aboutFeedback.css
@@ -94,5 @@
>  
>  @media not screen and (max-height: 400px) {
> -  body {
> -    padding-top: 40px;
> -  }
> Why did you get rid of this padding?

With this padding, the submit button overlaps with the help links we provide.
Attachment #8469666 - Attachment is obsolete: true
Attachment #8474109 - Flags: feedback?(margaret.leibovic)
Attached image mock.png
Screenshot for the updated patch.

@antlam: I have used mock images for the help section image and for thank you page. I would like the icons for the same.
Flags: needinfo?(alam)
Comment on attachment 8474109 [details] [diff] [review]
revamp_feedback.patch

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

Structurally, this looks like it's on the right track. We should move onto getting the right colors/fonts/spacing. I think we can still make progress on that while we wait for the real icons from antlam, although we will want to ask him for final values for things like colors/padding/margin.

::: mobile/android/chrome/content/aboutFeedback.js
@@ +37,5 @@
>    }
>  
>    let sumoLink = Services.urlFormatter.formatURLPref("app.support.baseURL");
> +  document.getElementById("sumo-link1").href = sumoLink;
> +  document.getElementById("sumo-link2").href = sumoLink;

Instead of using these two different ids, you could use a class name, then use getElementsByClassName to get all the elements with this class name and iterate through them to set the correct link.

::: mobile/android/locales/en-US/chrome/aboutFeedback.dtd
@@ +17,5 @@
>  <!-- LOCALIZATION NOTE (support.pre): Include a trailing space as needed. -->
>  <!-- LOCALIZATION NOTE (support.link): Avoid leading/trailing spaces, this text is a link. -->
>  <!-- LOCALIZATION NOTE (support.post): Include a starting space as needed. -->
> +<!ENTITY support.pre3              "Visit our ">
> +<!ENTITY support.link2             "Help Section">

I know this was in antlam's mock-up, but "Help Section" doesn't quite sound right to me, since this is taking the user to our support website, not a "section" of something. What about "Support Site"?

@@ +18,5 @@
>  <!-- LOCALIZATION NOTE (support.link): Avoid leading/trailing spaces, this text is a link. -->
>  <!-- LOCALIZATION NOTE (support.post): Include a starting space as needed. -->
> +<!ENTITY support.pre3              "Visit our ">
> +<!ENTITY support.link2             "Help Section">
> +<!ENTITY support.post3             " to get answers on common issues.">

Also feedback for antlam: the grammar here sounds off. I would say this as "to get answers for common issues".
Attachment #8474109 - Flags: feedback?(margaret.leibovic) → feedback+
(In reply to :Margaret Leibovic from comment #32)
> Also feedback for antlam: the grammar here sounds off. I would say this as
> "to get answers for common issues".

Works for me! :)

(In reply to vivek from comment #31)
> Created attachment 8474110 [details]
> mock.png
> 
> Screenshot for the updated patch.
> 
> @antlam: I have used mock images for the help section image and for thank
> you page. I would like the icons for the same.

I'm noticing some weird cut off where the content is on the right-most edge. In particular, the "can" looks like "car". 

As for the images, I can get those to you.
Flags: needinfo?(alam)
Attached patch revamp_feedback_v2.patch (obsolete) — Splinter Review
Please find the patch updated with icons provided.

@Margaret, 
1) Do you think the submit button should be disabled at the beginning?
2) Should we hide the entire url div along with the checkbox in case of empty url?
Attachment #8474109 - Attachment is obsolete: true
Attachment #8477059 - Flags: feedback?(margaret.leibovic)
Attached image mock_updated.png
Mock of the updated patch.

@antlam: can you please provide me with the color values for the feedback button in enabled state.

Also, can you please give me the color values for help section background.
Attachment #8477062 - Flags: feedback?(alam)
Drive-by design feedback: the footer image (for the support bit) is kinda competing for attention with the main page content. I'd probably make smaller and/or greyscale. antlam, thoughts?
Flags: needinfo?(alam)
Comment on attachment 8477062 [details]
mock_updated.png

These images are way to big here I think you might have used the XXHDPI ones on a XHDPI screen or I might have to look at shrinking them for you.

As per the colors, you can probably sample them from the design up there in comment 16 :).

But, here they are as well, #0092DB (blue), #FAFAFA (background [not inputbox])
Attachment #8477062 - Flags: feedback?(alam) → feedback-
(In reply to Lucas Rocha (:lucasr) from comment #37)
> Drive-by design feedback: the footer image (for the support bit) is kinda
> competing for attention with the main page content. I'd probably make
> smaller and/or greyscale. antlam, thoughts?

Yeah - looking back at this now and it seems so. I tried making that text Roboto Light rather than "regular" and that already helps. Also, "regular" in design files and "regular" in practice seem very different (in practice it looks like Light).

If I remember correctly, the bottom section (although looks like a snippet) actually is meant to demand some attention because often there are answers there already for the feedback that the user might want to give. But I'd be happy to tweak this a little bit when it gets closer to being wrapped up :)
Flags: needinfo?(alam)
(In reply to vivek from comment #35)
> Created attachment 8477059 [details] [diff] [review]
> revamp_feedback_v2.patch
> 
> Please find the patch updated with icons provided.
> 
> @Margaret, 
> 1) Do you think the submit button should be disabled at the beginning?

I think we should have it be disabled at the beginning, then enable it as soon as a character is typed. That's what the input.mozilla.org web interface does: https://input.mozilla.org/en-US/feedback#1

> 2) Should we hide the entire url div along with the checkbox in case of
> empty url?

Good question. In the case of an empty URL, I would say yes, we should hide the whole div, since there isn't really anything useful the user can do.

However, this makes me question the design a bit. antlam, did you realize that sometimes we wouldn't be able to get the correct last visited URL? It might be worth keeping the ability to edit the URL.
(In reply to :Margaret Leibovic from comment #40)
> (In reply to vivek from comment #35)
> > Created attachment 8477059 [details] [diff] [review]
> > revamp_feedback_v2.patch
> > 
> > Please find the patch updated with icons provided.
> > 
> > @Margaret, 
> > 1) Do you think the submit button should be disabled at the beginning?
> 
> I think we should have it be disabled at the beginning, then enable it as
> soon as a character is typed. That's what the input.mozilla.org web
> interface does: https://input.mozilla.org/en-US/feedback#1
> 
> > 2) Should we hide the entire url div along with the checkbox in case of
> > empty url?
> 
> Good question. In the case of an empty URL, I would say yes, we should hide
> the whole div, since there isn't really anything useful the user can do.
> 
> However, this makes me question the design a bit. antlam, did you realize
> that sometimes we wouldn't be able to get the correct last visited URL? It
> might be worth keeping the ability to edit the URL.

But on the other hand, the user can always just type a problematic site into the text area, so maybe we don't need to worry about this.
(In reply to vivek from comment #30)
> Created attachment 8474109 [details] [diff] [review]
> revamp_feedback.patch
> 
> A new patch which incorporates the reviews from mfinkle & margaret .
> 
> ::: mobile/android/themes/core/aboutFeedback.css
> @@ -94,5 @@
> >  
> >  @media not screen and (max-height: 400px) {
> > -  body {
> > -    padding-top: 40px;
> > -  }
> > Why did you get rid of this padding?
> 
> With this padding, the submit button overlaps with the help links we provide.

This makes me worried that these styles are not very robust. We'll want to test with different screen sizes/resolutions and font sizes. I think you should make sure the page scrolls if things don't all fit.
Comment on attachment 8477059 [details] [diff] [review]
revamp_feedback_v2.patch

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

This still seems like it's on the right track, but we need to fix the image scaling issues, and I'd also like to see use remove hard-coded pixel values. When writing these styles, think about how they will look on all sort of different screen sizes/resolutions. This will need to look alright on tablets in addition to phones.

Also, I want to test this on multiple different devices and resolutions. Perhaps you can try using the emulator for that? I can also help test on my devices once we get a version that is looking like the mock-up.

::: mobile/android/chrome/content/aboutFeedback.js
@@ +37,5 @@
>    }
>  
>    let sumoLink = Services.urlFormatter.formatURLPref("app.support.baseURL");
> +  document.getElementsByClassName("sumo-link")[0].href = sumoLink;
> +  document.getElementsByClassName("sumo-link")[1].href = sumoLink;

This makes an assumption that there are exactly 2 links with this class name. A more robust way to do thing would be something like:

let links = document.getElementsByClassName("sumo-link");
for (let link of links) {
  link.href = sumoLink;
}

@@ +107,3 @@
>  
> +  // Only send a URL string if the user wishes to.
> +  if (urlCheckBox.checked && urlElement.validity.valid) {

If the user is no longer entering/editing the URL themselves, we don't need this validity check.

@@ +110,2 @@
>  	data.append("add_url", true);
>  	data.append("url", urlElement.value);

Nit: While you're in here, you could fix this indentation to be 2 spaces.

::: mobile/android/chrome/content/aboutFeedback.xhtml
@@ +62,5 @@
>        <textarea class="description" placeholder="&sad.placeholder;" rows="8" required="true"/>
> +      <div class="message">
> +        <span>&sad.lastSite2;</span>
> +        <input id="last-checkbox" type="checkbox"/>
> +        <input id="last-url" type="url" placeholder="&sad.urlPlaceholder;" readonly="readonly"/>

If the user can't edit the URL, this doesn't need to be an input anymore. You can also get rid of the placeholder string.

::: mobile/android/themes/core/aboutFeedback.css
@@ +61,5 @@
>    background-color: #a7b0b8;
>  }
>  
>  .message {
> +  padding-right: 1px;

Why was this padding added? It feels odd that there's just 1px on one side.

@@ +113,5 @@
> +}
> +
> +@media screen and (min-resolution: 1dppx) {
> +  .sumo-icon {
> +    background-image: url("chrome://browser/skin/images/icon_floaty_mdpi.png");

Instead of using background images, have you tried just using img elements? I don't really see a reason for using background images, since these images are basically just standalone elements.

You can set different image sizes in JS for different screen resolutions. Here's an example of that:
https://github.com/leibovic/fennec_rss/blob/master/bootstrap.js#L344

@@ +118,5 @@
> +    position: relative;
> +    float: left;
> +    padding-right: 10px;
> +    width: 41px;
> +    height: 41px;

This hard-coded values make me nervous. I think we should find a way to make these elements take up the right amount of space without a hard-coded size.

@@ +119,5 @@
> +    float: left;
> +    padding-right: 10px;
> +    width: 41px;
> +    height: 41px;
> +    background-repeat: no-repeat;

Styles that are the same for all resolutions should be pulled out of any media queries.

@@ +125,5 @@
> +
> +  #sad-thanks-icon {
> +    background-image: url("chrome://browser/skin/images/icon_heart_mdpi.png");
> +    position: relative;
> +    left: -moz-calc(50% - 30px);

Where do the values in this calculations come from? I think that we should just center the image in the page, rather than set a hard-coded left value.

@@ +134,5 @@
> +    background-position: center;
> +  }
> +}
> +
> +@media screen and (min-resolution: 2dppx) {

You're missing hdpi styles in here. Maybe that's what caused the scaling issues we're seeing in your screenshots?

@@ +200,4 @@
>  }
>  
> +.sumo-icon {
> +  background-image: url("chrome://browser/skin/images/icon_floaty_hdpi.png");

mdpi should be the default, not hdpi. See the styles and media queries in this file as an example:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css#491

@@ +241,5 @@
> +#last-checkbox:not(:checked) ~ #last-url {
> +  visibility: hidden;
> +}
> +
> +#sad-thanks-icon {

I think that you should be putting the default values above the media queries, not below them.
Attachment #8477059 - Flags: feedback?(margaret.leibovic) → feedback+
Margaret,
The description field is restricted to max of 10000 char in 
https://input.mozilla.org/en-GB/feedback#1

we should do the same?

antlam,
if we do that we should also show the no. of characters left?
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(alam)
(In reply to vivek from comment #44)
> antlam,
> if we do that we should also show the no. of characters left?

That's cool :) but I'm not too worried about that for now. At the time of this design, I don't think we were often getting feedback that was near 1000 characters long. 

We can iterate on this as we need of course but due to the lack of space, this might be harder to wedge in. I'd like to get it on par with the current mocks first.
Flags: needinfo?(alam)
(In reply to vivek from comment #44)
> Margaret,
> The description field is restricted to max of 10000 char in 
> https://input.mozilla.org/en-GB/feedback#1

10000 characters is really huge, especially for a mobile device! I don't think we need to worry too much about this, since we don't currently have a max in place. Since this bug is just about updating the styling, let's stay focused on that, and we can work on a max character count in a separate bug if it becomes a problem.
Flags: needinfo?(margaret.leibovic)
Attached patch revamp_feedback.patch (obsolete) — Splinter Review
A new patch with the following changes

-Corrected the previous review comments.
-Fixed the help section to bottom of the screen as per the discussion over irc.
-Submit button states handled.
Attachment #8477059 - Attachment is obsolete: true
Attachment #8485317 - Flags: review?(margaret.leibovic)
Attached image Mock.png
A mock up for the new patch.

@antlam : Is the color gradient for submit button ok?

@Margaret: having the help section fixed at the bottom of the screen is problematic when the keyboard pops up (see the fig 4).
Attachment #8485319 - Flags: review?(margaret.leibovic)
Attachment #8485319 - Flags: feedback?(alam)
Comment on attachment 8485319 [details]
Mock.png

Hmm.. it's interesting but I think it's best to stick to the visual language we've been establishing recently. The gradient doesn't seem necessary to me since it's pretty obvious what the call-to-action/ button is here. :)
Attachment #8485319 - Flags: feedback?(alam) → feedback-
(In reply to vivek from comment #48)
> Created attachment 8485319 [details]
> Mock.png
> 
> A mock up for the new patch.
> 
> @antlam : Is the color gradient for submit button ok?
> 
> @Margaret: having the help section fixed at the bottom of the screen is
> problematic when the keyboard pops up (see the fig 4).

Hm, that fixed footer is definitely a challenge. I wonder if we can just hide it when the keyboard is up. I'm not sure if we have a way to tell the keyboard is up in JS, but perhaps we could use media queries to hide it if the available height is too small.

antlam, what do you think we should do here?
Comment on attachment 8485317 [details] [diff] [review]
revamp_feedback.patch

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

Looking good! We just need to continue to iterate to match antlam's mock-ups. The biggest outstanding issue is figuring out the footer behavior, but we also need to refine some background colors. It also looks like the heart icon is too big, I wonder if something is going wrong with the JS logic for setting that image. The floaty icon also looks a bit blurry to me.

::: mobile/android/chrome/content/aboutFeedback.js
@@ +47,5 @@
> +    helpSectionIcon = FLOATY_ICON_XHDPI;
> +    sadThanksIcon = HEARTY_ICON_XHDPI;
> +  } else {
> +    helpSectionIcon = FLOATY_ICON_XXHDPI;
> +    sadThanksIcon = HEARTY_ICON_XXHDPI;

I wonder if this JS isn't working as well as we had hoped. Maybe we should try using media queries again.

@@ +56,5 @@
> +
> +  sadThanksImg.src = sadThanksIcon;
> +  for (let link of sumoIcons) {
> +    link.href = helpSectionIcon;
> +  }

Now that you created a single footer element, you don't need this for loop, and you could use an id instead of a class name.

Also, this seems wrong, I think you meant to set an image src here, not an href.

@@ +68,5 @@
> +	    feedbackButton.disabled = false;
> +	  } else {
> +		feedbackButton.disabled = true;
> +	  }
> +	}, false);

Looks like there are some indentation issues here. Please make sure you just use 2-space indentation in JS.

Also, I think this is out of the scope of this bug. Let's just maintain the current behavior, and we can file a separate bug to deal with disabling the button.

::: mobile/android/chrome/content/aboutFeedback.xhtml
@@ +61,5 @@
>        <div class="message">&sad.message;</div>
>        <textarea class="description" placeholder="&sad.placeholder;" rows="8" required="true"/>
> +      <div class="message">
> +        <span>&sad.lastSite2;</span>
> +        <input id="last-checkbox" type="checkbox"/>

I think we should default this to be checked.

@@ +79,5 @@
>      </div>
>    </section>
>  
> +  <footer>
> +    <img class="sumo-icon" src="chrome://browser/skin/images/icon_floaty_mdpi.png" />

You should omit the src attributes here if you're going to set them dynamically, since it could hide bugs if the JS isn't working properly.

::: mobile/android/themes/core/aboutFeedback.css
@@ +31,5 @@
> +  display: none;
> +  color: #444;
> +  padding-top: 20px;
> +  padding-bottom: 10px;
> +  max-width: 500px;

What is this max width for? I think it will look strange if the footer doesn't cross the entire screen.

@@ +34,5 @@
> +  padding-bottom: 10px;
> +  max-width: 500px;
> +  margin-left: auto;
> +  margin-right: auto;
> +  height: 10%;

I think we should give the footer a pixel value for height. Otherwise this might look odd on different screen sizes (imagine a tablet in portrait mode).

@@ +37,5 @@
> +  margin-right: auto;
> +  height: 10%;
> +  position: fixed;
> +  bottom: 0px;
> +}

We should be able to set a background color and top border on this element to match antlam's mock-ups.

@@ +78,5 @@
>  
>  .message {
>    margin-bottom: 10px;
> +  margin-left: auto;
> +  margin-right: auto;

Glad you got this to work :)

@@ +108,5 @@
>  
> +.sumo-icon {
> +  position: relative;
> +  float: left;
> +  padding-right: 10px;

This padding might cause the image to scale. Could you try margin instead?

@@ +113,5 @@
>  }
>  
> +#sad-thanks-icon {
> +  position: relative;
> +  display: block;

Do you need this display: block? Isn't that the default?

@@ +175,4 @@
>    padding: 10px;
>    font-size: 16px;
>    width: 100%;
> +  background-image: linear-gradient(rgb(0,146,219), rgb(0,130,200) 90%, rgb(0,111,184));

Instead of a background image, you should be able to just set a backgroud-color to match the solid color antlam used in the mock-ups.
Attachment #8485317 - Flags: review?(margaret.leibovic) → feedback+
Attachment #8485319 - Flags: review?(margaret.leibovic)
Attached patch revamp_feedback.patch (obsolete) — Splinter Review
Updated intermediate patch.

1) Feedback from previous patch corrected
2) footer hidden during virtual keyboard.

@Margaret : I've corrected the image selection logic. But HDPI images seems to be the default in my nexus-s. can you please check with some other device.

Also, the footer is show/hidden based on focus/blur event on textarea. However, it is not visible again when I dismiss the keyboard through h/w back button.
Attachment #8485317 - Attachment is obsolete: true
Attachment #8486825 - Flags: review?(margaret.leibovic)
Attached image Mocks (obsolete) —
@Margaret: what should be height for the footer? It depends on the image height right?
Attachment #8486826 - Flags: review?(alam)
Attachment #8486826 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8486826 [details]
Mocks

It's coming along! Just some quick feedback.

I'm not sure you're using the right icon sizes here, they seem like they're a size up from what they should be.

The background color is also not the right color, could you check comment 19 and above for the right colors here?
Attachment #8486826 - Flags: review?(alam) → review-
Comment on attachment 8486826 [details]
Mocks

I agree with antlam's points.

(In the future, you can just request review from him on the screenshots, since he's in charge of the visuals. I can just review the code :)
Attachment #8486826 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8486825 [details] [diff] [review]
revamp_feedback.patch

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

I'm clearing review for now. Let's first try to get this to match antlam's mock-ups, and then I'll review the code (otherwise it's a bit tedious to keep going through all the code changes every time). But let me know if there are any specific bits that you are having trouble implementing, and I can help you with those details.
Attachment #8486825 - Flags: review?(margaret.leibovic)
Attached patch revamp_feedback.patch (obsolete) — Splinter Review
A new patch that does the following
* Corrected background color. 
* Fixed the footer.
Attachment #8486825 - Attachment is obsolete: true
Attachment #8491228 - Flags: review?(margaret.leibovic)
Attached file mock.zip (obsolete) —
@antlam: should I change bg for message box?
Attachment #8486826 - Attachment is obsolete: true
Attachment #8491230 - Flags: review?(alam)
(In reply to vivek from comment #58)
> Created attachment 8491230 [details]
> mock.zip
> 
> @antlam: should I change bg for message box?

Awesome work Vivek!

Yes, let's try making that white (#FFFFFF). The bluish grey doesn't match anymore.

We could then use #EBEBF0 for the bottom part to clean that up too :)
Attachment #8491230 - Flags: review?(alam) → review-
Attached file Mock_updated.zip (obsolete) —
New mocks with the bg color updated.
Attachment #8491230 - Attachment is obsolete: true
Attachment #8494134 - Flags: review?(alam)
Attached patch revamp_feedback.patch (obsolete) — Splinter Review
New patch after #1066062 merge. 
Also, the bg color of the link-box has been updated w.r.t to antlam comment.
Attachment #8491228 - Attachment is obsolete: true
Attachment #8491228 - Flags: review?(margaret.leibovic)
Attachment #8494135 - Flags: review?(margaret.leibovic)
Comment on attachment 8494134 [details]
Mock_updated.zip

This is definitely on it's way, Vivek. 

Please now refer to comment 16 for the proper padding and margins to complete the visual structure of the page. :)
Attachment #8494134 - Flags: review?(alam) → review-
Attached file Mock.zip (obsolete) —
New Mock images with padding and margins updated.
Attachment #8494134 - Attachment is obsolete: true
Attachment #8496110 - Flags: review?(alam)
Attached patch revamp_feedback_styling.patch (obsolete) — Splinter Review
A new patch with style changes.
Attachment #8494135 - Attachment is obsolete: true
Attachment #8494135 - Flags: review?(margaret.leibovic)
Attachment #8496111 - Flags: review?(margaret.leibovic)
Comment on attachment 8496110 [details]
Mock.zip

Awesome work!

Just three things I'd like to fix about the visuals.

1) the links on the "intro" page, could we have those as #0092DB? Right now they're still the old dark blue I believe. 

2) the footer on "sad" where we have "For your privacy, please do not include...", could we bump that up to the same font size as the "include last visited site"?

3) The footer's containing element height should be reduced to 80 dp. The links inside this should be the same color as the copy also, just underlined :)
Attachment #8496110 - Flags: review?(alam) → review-
Flags: needinfo?(vivekb.balakrishnan)
Attached file mock.zip (obsolete) —
A new patch with 
* link color change
* footer height fixed to 80px
Attachment #8496110 - Attachment is obsolete: true
Attachment #8496281 - Flags: review?(alam)
Flags: needinfo?(vivekb.balakrishnan)
Attached patch revamp_feedback_styling.patch (obsolete) — Splinter Review
patch for the corresponding mock.
Attachment #8496111 - Attachment is obsolete: true
Attachment #8496111 - Flags: review?(margaret.leibovic)
Attachment #8496283 - Flags: review?(margaret.leibovic)
Comment on attachment 8496281 [details]
mock.zip

Hey Vivek, just a couple things - this is so close!

From our conversation on IRC, you've got most of it covered but the "it takes less than a minute" copy is still the light grey when it should be matching the body font color?

Also, did you manage to make the change to the footer element? it still looks the same height.

The empty state for the URL is also weird in the new screen shots you attached. if there's no URL it shouldn't even have "http://"

Finally, for the "haapy" screen, where are we getting that 5 star image? the box around it seems weird to me. We should probably get rid of that too.

Almost there! :)
Attachment #8496281 - Flags: review?(alam) → review-
Attached file mock.zip (obsolete) —
A new mock with the following changes
* background for stars removed
* font color changes in positive feedback form
* Disabled checkbox in negative feedback form when the url is not valid.

@antlam: 

1) I've added screenshots from web inspector. The footer content is 80px in height with 2px border.

The div inside the footer has content height 41px with 5px padding on all sides. The margin-top and bottom for this div are 14px and 15px respectively .

2) Should the entire "include last visited url" be disabled in the case where the url is empty?
Attachment #8496281 - Attachment is obsolete: true
Attachment #8496745 - Flags: review?(alam)
Attached patch revamp_feedback_styling.patch (obsolete) — Splinter Review
Updated patch for the corresponding mocks
Attachment #8496283 - Attachment is obsolete: true
Attachment #8496283 - Flags: review?(margaret.leibovic)
Attachment #8496746 - Flags: review?(margaret.leibovic)
(In reply to vivek from comment #69)
> Created attachment 8496745 [details]
> mock.zip
> 
> A new mock with the following changes
> * background for stars removed
> * font color changes in positive feedback form
> * Disabled checkbox in negative feedback form when the url is not valid.
> 
> @antlam: 
> 
> 1) I've added screenshots from web inspector. The footer content is 80px in
> height with 2px border.
> 
> The div inside the footer has content height 41px with 5px padding on all
> sides. The margin-top and bottom for this div are 14px and 15px respectively
> .

I'll attach the design again after this but the space on these screenshots I'm seeing seems extremely large.

> 2) Should the entire "include last visited url" be disabled in the case
> where the url is empty?

Hm.. Not sure what use-case this would be. Won't all our pages always have some sort of link associated with them? Coincidentally, the "off" state for this checkbox should be a white filled box (the one im seeing in the screenshots has a brown filled inside). Is your box 20dp sq? it also seems a bit small.

Also, just wanted to make sure the "Send Feedback" button will be #8A9BA8 if it's inactive? from your screen shots it seems that it's blue (active) all the time.
Hey Vivek,

Maybe it's the font size? I have 16sp in the design.
(In reply to Anthony Lam (:antlam) from comment #72)
> Created attachment 8497178 [details]
> About:feedback confirmation.png
> 
> Hey Vivek,
> 
> Maybe it's the font size? I have 16sp in the design.

I don't think we can use sp because this is web content, not native UI. I think the fact that this is web content is what's leading to some of the confusion over the design, since we can't specify dp and sp like we would in native UI.

I'm not sure what designers for the web do (maybe some folks from the UX team have experience with this?), but we'll need to just follow web development practices here.

We currently use px for font sizes:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutFeedback.css#8

So I think we just need to find some px values we're happy with.
Comment on attachment 8496746 [details] [diff] [review]
revamp_feedback_styling.patch

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

This is looking really good! I just have a few coding style comments, but on the technical side this patch should be ready to land once we're happy with the styles.

I just found 2 small style issues while testing this on my phone, and I'll upload screenshots to show you what I mean.

Other than that, I think we should try to land this patch, and address any other issues we find in follow-up bugs. This is a big improvement, and we've been iterating on this for long enough!

::: mobile/android/chrome/content/aboutFeedback.js
@@ +61,5 @@
>    window.addEventListener("unload", uninit, false);
>  
> +  let description = document.getElementsByClassName("description")[0];
> +  description.addEventListener("focus", function(evt) {
> +    document.querySelector('footer').style.visibility="hidden";

I think it would be better to do this by setting an attribute on the element, then setting the actual style in the CSS.

Something like

footer[hidden] {
  visibility: hidden;
}

Then you can just remove the attribute in the blur event handler.

@@ +84,5 @@
>  
>    // Fill "Last visited site" input with most recent history entry URL.
>    Services.obs.addObserver(function observer(aSubject, aTopic, aData) {
>  	document.getElementById("last-url").value = aData;
> +	// Enable the checkbox in case the URL is valid.

Please fix the indentation here to be 2 spaces (you can do this to the line above as well).

@@ +88,5 @@
> +	// Enable the checkbox in case the URL is valid.
> +    if (aData.length != 0) {
> +      document.getElementById("last-checkbox").disabled = false;
> +      document.getElementById("last-checkbox").checked = true;
> +    }

If there's no URL, showing an empty line and empty checkbox seems odd. I think we should just hide the whole element if there's no URL.

And we can default the checkbox to checked and enabled in the HTML.

::: mobile/android/chrome/content/aboutFeedback.xhtml
@@ +83,5 @@
> +    <div id="help-section">
> +      <img id="sumo-icon" />
> +      <span>&support.pre3;<a id="sumo-link">&support.link2;</a>&support.post3;</span>
> +    </div>
> +  </footer>

We can address this in a follow-up bug, but I think we should make the whole footer a click target, and I think we should open the support page in a new tab, in case the user still wants to leave feedback. Can you file a follow-up bug about refining this banner behavior?

::: mobile/android/themes/core/aboutFeedback.css
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +html {
> +  height: 100%;

Why is this needed?

@@ +11,5 @@
>    font-family: sans-serif;
>    font-size: 12px;
>    color: #222;
> +  background-color: #fafafa;
> +  height: 100%;

Same for this, shouldn't this be the default?

@@ +138,5 @@
>  }
>  
> +#thanks-sad[active="true"] ~ footer {
> +  display: block;
> +}

Nit: I would put these style declarations up by the footer style.
Attachment #8496746 - Flags: review?(margaret.leibovic) → feedback+
I think we need to look into some of the margin/padding styles, because the content is getting cut off when the keyboard is open.
This screenshot looks different than what's actually displayed on my phone, but on my phone I don't see a difference between the button color and the background color. Maybe we can address this in a follow-up bug, but it looks pretty bad on my device. To fix this I think we'd need to specify a different color for the buttons.
A new patch with the nits corrected.

@margaret:
As discussed over irc, I've used media query to hide the footer when the viewport is small. I'll attach a mock zip shortly.
Attachment #8496746 - Attachment is obsolete: true
Attachment #8499073 - Flags: review?(margaret.leibovic)
Attached file mocks.zip
A new mock zip for the patch.

zip contain screenshots for all the section in both landscape and portrait orientation
Attachment #8496745 - Attachment is obsolete: true
Attachment #8496745 - Flags: review?(alam)
Attachment #8499075 - Flags: review?(alam)
Blocks: 1077047
Comment on attachment 8499073 [details] [diff] [review]
revamp_feedback_styling.patch

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

Excellent! I'm happy the media query worked. That also appears to fix the cut-off content problem I was seeing.

Let's get sign-off from antlam, but I'd be happy to land this.

::: mobile/android/themes/core/aboutFeedback.css
@@ +6,5 @@
>    -moz-text-size-adjust: none;
>    font-family: sans-serif;
>    font-size: 12px;
>    color: #222;
> +  background-color: #fafafa;

I'm not sure if it's a Fennec bug, but maybe this #fafafa value is just too close to white, so it's rendering as white on my device. Updating this to #f0f0f0 fixes this for me locally, and I think that would be close enough.

I'll check with antlam to see what he thinks, but seeing the background appear white on my device does not look good.
Attachment #8499073 - Flags: review?(margaret.leibovic) → review+
Blocks: 1077080
Comment on attachment 8499075 [details]
mocks.zip

+ for most of it! :)

(In reply to :Margaret Leibovic from comment #73)
> So I think we just need to find some px values we're happy with.

I use the same px value (48px in my design) for the footer text as the "Send feedback" button so maybe we could try that?

> If there's no URL, showing an empty line and empty checkbox seems odd. I
> think we should just hide the whole element if there's no URL.
> 
> And we can default the checkbox to checked and enabled in the HTML.

I think we still need it, the absence of it when it's not checked gives the user no way to get back to it if they change their mind (either before or after seeing the URL they're actually sharing). This way, it can function more like a toggle.

Thoughts, Margaret?

> We can address this in a follow-up bug, but I think we should make the whole
> footer a click target, and I think we should open the support page in a new
> tab, in case the user still wants to leave feedback. Can you file a
> follow-up bug about refining this banner behavior?

+1 :)
Attachment #8499075 - Flags: review?(alam) → review+
Flags: needinfo?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #79)
> I'm not sure if it's a Fennec bug, but maybe this #fafafa value is just too
> close to white, so it's rendering as white on my device. Updating this to
> #f0f0f0 fixes this for me locally, and I think that would be close enough.
> 
> I'll check with antlam to see what he thinks, but seeing the background
> appear white on my device does not look good.

#f5f5f5! that's the same color as our header banners - so it should be good. I was worried about #FAFAFA but I think it was something Ian and I were trying back then.
(In reply to Anthony Lam (:antlam) from comment #80)

> > If there's no URL, showing an empty line and empty checkbox seems odd. I
> > think we should just hide the whole element if there's no URL.
> > 
> > And we can default the checkbox to checked and enabled in the HTML.
> 
> I think we still need it, the absence of it when it's not checked gives the
> user no way to get back to it if they change their mind (either before or
> after seeing the URL they're actually sharing). This way, it can function
> more like a toggle.
> 
> Thoughts, Margaret?

My point was that we should hide the whole element that contains the "Include last visited site"/URL/checkbox when there is no URL. If there's no URL, we have nothing to show and nothing to submit, so a checkbox would be confusing. This is more of an error-handling case than anything else.
Flags: needinfo?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #82)
> My point was that we should hide the whole element that contains the
> "Include last visited site"/URL/checkbox when there is no URL. If there's no
> URL, we have nothing to show and nothing to submit, so a checkbox would be
> confusing. This is more of an error-handling case than anything else.

I see what you're saying now. My mistake, I misunderstood and thought it was for everything rather than just the pages with "no URL". 

To sum up, if the page has a URL and the user chooses not to share it, the checkbox stays, but if the page has NO URL, the box wouldn't even appear. 

This makes sense to me - but the padding should also adjust accordingly rather than leave a void. Vivek, could we also fix that? :)
Flags: needinfo?(vivekb.balakrishnan)
A new patch with the comment incorporated.

@antlam,
I have set the footer text to be same size as submit button & padding in case of empty url has been corrected

Please find the mock uploaded at
https://docs.google.com/file/d/0B34Nogao7VfpVUt3ckpkbFFHQlk/edit
Attachment #8499766 - Flags: review?(margaret.leibovic)
Attachment #8499766 - Flags: feedback?(alam)
Flags: needinfo?(vivekb.balakrishnan)
Comment on attachment 8499766 [details] [diff] [review]
revamp_feedback_styling.patch

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

Looks good! I pushed this to try to make sure nothing unexpected is broken, and we can land this on fx-team if everything is green.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fae623607f00

At this point, I think we should just land this as it is, and we can continue to file new bugs for any other improvements we want to make.
Attachment #8499766 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8499766 [details] [diff] [review]
revamp_feedback_styling.patch

looks good! + to land!

Great work Vivek, it's been a long bug :P
Attachment #8499766 - Flags: feedback?(alam) → feedback+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/02543de24c5c
Keywords: checkin-needed
Whiteboard: [lang=html][lang=js][lang=css] → [lang=html][lang=js][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/02543de24c5c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=html][lang=js][lang=css][fixed-in-fx-team] → [lang=html][lang=js][lang=css]
Target Milestone: --- → Firefox 35
Depends on: 1093119
Verified as fixed in latest builds:
Firefox for Android 37.0a1 (2014-12-09)
Firefox for Android 36.0a2 (2014-12-09)
Firefox for Android 35.b1

Device: Nexus 4 (Android 4.4.4)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.