Remove gradients from form elements

RESOLVED FIXED in Firefox 33

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: ibarlow, Assigned: wesj)

Tracking

(Depends on: 1 bug, Blocks: 5 bugs, {feature})

15 Branch
Firefox 35
ARM
Android
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, firefox35 fixed, relnote-firefox 33+, fennec34+)

Details

(Whiteboard: ui-hackathon)

Attachments

(3 attachments, 19 obsolete attachments)

99.58 KB, image/png
Details
12.44 KB, text/html
Details
24.10 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Our form elements look pretty out of date, so we should update them to better fit with our new design aesthetic. (https://twitter.com/cwiiis/status/211573240836005888)

This is the bug to track that progress, and we should include

* pulldown menus
* input fields
* text fields
* radio buttons
* checkboxes

I'll post some new designs soon.
(Reporter)

Comment 1

5 years ago
New designs! http://cl.ly/image/441g2z2X3p0r
(Assignee)

Comment 2

4 years ago
Created attachment 720974 [details] [diff] [review]
Patch 1/2

Mostly there, but still some annoying issues with the file upload. I'll post a screenshot/build and get some feedback from ian and mark.
Attachment #720974 - Flags: feedback?(mark.finkle)
(Assignee)

Comment 3

4 years ago
Created attachment 720978 [details] [diff] [review]
Patch 2/2

Adds prefs to disable drawing checkmarks or radio buttons in layout. I think I need to name these better, but wanted to make sure this approach was ok with dbaron. In the long run, I've got a WIP at implementing nsNativeTheme for Android, but its quite a ways from being done and this seems useful for now.
Assignee: nobody → wjohnston
Attachment #720978 - Flags: feedback?(dbaron)
(Assignee)

Comment 4

4 years ago
Created attachment 720981 [details]
Screenshot old vs new

Build is at:

http://dl.dropbox.com/u/72157/fennec-forms.apk
Flags: needinfo?(ibarlow)
(Assignee)

Comment 5

4 years ago
My test page is at http://dl.dropbox.com/u/72157/forms.html
(Reporter)

Comment 6

4 years ago
Omg. 

This looks excellent. Let's ship it! \o/
Flags: needinfo?(ibarlow)
(Reporter)

Comment 7

4 years ago
Actually... Mark just made a good point, the disabled text input looks very similar to disabled buttons. 

I wonder if the disabled input fields can be tweaked somewhat, perhaps with a white background and light grey text / border
Comment on attachment 720974 [details] [diff] [review]
Patch 1/2


>diff --git a/mobile/android/themes/core/content.css b/mobile/android/themes/core/content.css
>+input[type="submit"]::-moz-focus-inner,
>+input[type="file"] > input[type="button"]::-moz-focus-inner {
>+  padding: 0;
>+  border: none;
> }
> 
>-xul|window xul|scrollbarbutton[sbattr="scrollbar-up-top"],
>-xul|window xul|scrollbarbutton[sbattr="scrollbar-bottom-top"] {
>-  display: none;
>+
>+button:disabled:active, button:disabled,

Looks like we end up with two blank lines here

>diff --git a/mobile/android/themes/core/images/icons.svg b/mobile/android/themes/core/images/icons.svg

Can we rename to form-icons.svg ?
>+.outline {
>+    fill:none;
>+    stroke:#777980;
>+    stroke-width:2.7;
>+    stroke-miterlimit:4;

Use space after ':'

>+.fill {
>+    fill:url(#fillGradient);
>+    fill-opacity:1;
>+    stroke:none;

Same

>+#checkbox-fill {
>+    fill: url(#fillGradient);
>+    stroke:#ffffff;
>+    stroke-width:1.7;
>+    stroke-linecap:round;
>+    stroke-linejoin:round;
>+    stroke-miterlimit:4;

And so on

>+        <path d="m 6,10 l 8.5,8.75 l 8.5,-8.75"
>+           style="fill:none;stroke:url(#fillGradient);stroke-width:5;stroke-linecap:round;stroke-linejoin:round;stroke-miterlimit:4;"/>

And so forth

Visually, my only feedback is what Ian mentioned: disabled textboxes and normal buttons might be too similar
Attachment #720974 - Flags: feedback?(mark.finkle) → feedback+
(Assignee)

Comment 9

4 years ago
Finally tracked my file input woes to:

http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsFileControlFrame.cpp#598

which depends on styles in forms.css to be true. Fixing....
(Assignee)

Comment 10

4 years ago
Created attachment 722494 [details]
Revised disabled elements

Hows this ian? I also increased the strength of gradient up the buttons a bit (maybe too much).
grrr. gradients.
(Reporter)

Comment 12

4 years ago
(In reply to Wesley Johnston (:wesj) from comment #10)
> Hows this ian? I also increased the strength of gradient up the buttons a
> bit (maybe too much).

I think the button styles were fine the way you had them before, tbh. The new disabled fields look great, and the disabled text color on buttons is great. I would just revert the button backgrounds back to the version before this and we're good to go :)
(In reply to Ian Barlow (:ibarlow) from comment #12)
> (In reply to Wesley Johnston (:wesj) from comment #10)
> > Hows this ian? I also increased the strength of gradient up the buttons a
> > bit (maybe too much).
> 
> I think the button styles were fine the way you had them before, tbh. The
> new disabled fields look great, and the disabled text color on buttons is
> great. I would just revert the button backgrounds back to the version before
> this and we're good to go :)

Agreed
(Assignee)

Comment 14

4 years ago
Created attachment 722891 [details] [diff] [review]
Patch 2/2 - Theming

CSS changes. Moved back to the old button style and tweaked the dropmarker and checkboxes a little bit. One thing I haven't put in the screenshots because its kinda hard are active and hover states. I'll upload a build just in case ian wants to test them.

I also updated my test page to include a second set of form elements with a scoped stylesheet applied. We've got parity with desktop there and things look ok (even with a pretty ugly theme).
Attachment #720978 - Attachment is obsolete: true
Attachment #720978 - Flags: feedback?(dbaron)
Attachment #722891 - Flags: review?(mark.finkle)
(Assignee)

Comment 15

4 years ago
Created attachment 722892 [details] [diff] [review]
Patch 1/2 - Layout prefs

This adds the prefs I needed. If you know of a better solution, happy to hear!
Attachment #720974 - Attachment is obsolete: true
Attachment #722892 - Flags: review?(dbaron)
(Assignee)

Comment 16

4 years ago
Comment on attachment 722892 [details] [diff] [review]
Patch 1/2 - Layout prefs

Dholbert, if you're comfortable reviewing thins, feel free to steal.
Attachment #722892 - Flags: feedback?(dholbert)
Attachment #722891 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 17

4 years ago
Comment on attachment 722892 [details] [diff] [review]
Patch 1/2 - Layout prefs

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

Talked to dholbert about this in person. The issue I'm worried about here is that this will expose some theming abilities to content that they didn't have before. Namely, the ability to theme checkboxes or radios. I think that would make webdevs happy, but I'm sure there is a reason we didn't grant it before? If we are going to expose it, maybe a better way is to just check if the element has a background image set, and if so don't draw the checkmark?

dholbert's comment was that we should just implement nsNativeTheme which I'm working on, but I'm not convinced that we want all of it anyway. Android theme elements were not designed to blend well with the web. It will be interesting to see which ones work and which don't.
Attachment #722892 - Flags: feedback?(dholbert)
(Reporter)

Comment 18

4 years ago
(In reply to Wesley Johnston (:wesj) from comment #17)

> dholbert's comment was that we should just implement nsNativeTheme which I'm
> working on, but I'm not convinced that we want all of it anyway. Android
> theme elements were not designed to blend well with the web. It will be
> interesting to see which ones work and which don't.

Yeah I'm a little skeptical of how well that will work out. Open to seeing some screenshots, but if it makes the web look broken we shouldn't take that approach.
(Take my comments with a grain of salt: I haven't been involved with the design/bootstrapping of any other native themes, so I'm not sure what's involved with that.)

To expand slightly on wesj's paraphrased version of my comments: I'm wondering why we've never done anything like this for other platforms, and why android needs to be a special-case here.

This feels like it could be a slippery slope towards a situation where we've got N platforms, some subset of which have a "true" native theme, some subset of which use this background hack & C++-special-casing for some of its form controls, and some subset of which just use the cross-platform theme.  That's a complex landscape, and it seems like it'd be cleaner & more future-proof to make this fit into our existing framework.

(wesj said that there might be issues with getting actual remote android widgets into gecko, due to how android is set up. If that ends up being a barrier, I suspect we could implement a "native" theme that actually just draws static images, under the hood.)
(Assignee)

Comment 20

4 years ago
Created attachment 728492 [details] [diff] [review]
Updated patch

This is almost the same, but I ripped out the checkbox/radio/file input stuff for something much simpler. This doesn't override the layout styling and looks ok. There's a small issue with the file input controls that I can't quite pin down...

I also added some styling for input[type="range"] to get the ball rolling there. I'll post an updated screenshot.
Attachment #722891 - Attachment is obsolete: true
Attachment #728492 - Flags: review?(mark.finkle)
(Assignee)

Comment 21

4 years ago
Created attachment 728494 [details]
Screenshot

Oh yeah, I restyled the select elements to be a bit more androidy too (without going to far overboard.
Attachment #720981 - Attachment is obsolete: true
Attachment #722494 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 728498 [details] [diff] [review]
Updated patch again
Attachment #728492 - Attachment is obsolete: true
Attachment #728492 - Flags: review?(mark.finkle)
Attachment #728498 - Flags: review?(mark.finkle)
Comment on attachment 728498 [details] [diff] [review]
Updated patch again


>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+pref("layout.forms.radio.draw", false);
>+pref("layout.forms.checkmarks.draw", false);
>+pref("layout.forms.fileinput.draw.border", true);

Do you still need these?

What's up with the radios and checkboxes in the screenshot? The ones in the "black background" section are not sized correctly.

I am not too sure about the range thumb styling. I am glad you added it, but let's get some UX feedback on those and the <select> drop arrow buttons.

Otherwise looks good. I want to withhold r+ until we get an answer on the prefs and an OK on the styling from UX
Also, given this is the last week of the Fx22 cycle, I think we should land these in Fx23.
(Assignee)

Comment 25

4 years ago
(In reply to Mark Finkle (:mfinkle) from comment #23)
> Do you still need these?
No. will Remove. Thanks 

> What's up with the radios and checkboxes in the screenshot? The ones in the
> "black background" section are not sized correctly.

I have a input { width: 90%; } style rule in there which makes those do that. 
 
> I am not too sure about the range thumb styling. I am glad you added it, but
> let's get some UX feedback on those and the <select> drop arrow buttons.

I agree. I thought for a first draft I'd do something sorta Android like, but I"m not sure it fits will with these form elements. Likewise, after seeing it on a few sites, I'm still debating whether I love that new "select" style either.
(In reply to Mark Finkle (:mfinkle) from comment #24)
> Also, given this is the last week of the Fx22 cycle, I think we should land
> these in Fx23.

How will this work for <input type=range>? Can you guys decide on an interim design for Fx22 while these new designs are worked out? The other option is to flip the pref to turn off <input type=range> (dom.experimental_forms_range), but you'll also need to remove the applicable styling from forms.css since that styling isn't (can't be) behind that pref.
(In reply to Jonathan Watt [:jwatt] from comment #26)
> (In reply to Mark Finkle (:mfinkle) from comment #24)
> > Also, given this is the last week of the Fx22 cycle, I think we should land
> > these in Fx23.
> 
> How will this work for <input type=range>? Can you guys decide on an interim
> design for Fx22 while these new designs are worked out? The other option is
> to flip the pref to turn off <input type=range>
> (dom.experimental_forms_range), but you'll also need to remove the
> applicable styling from forms.css since that styling isn't (can't be) behind
> that pref.

I think we could land a "just <input type='range'>" patch for Fx22

Updated

4 years ago
Duplicate of this bug: 830851
(Reporter)

Updated

4 years ago
Whiteboard: ui-hackathon
(In reply to Mark Finkle (:mfinkle) from comment #27)
> I think we could land a "just <input type='range'>" patch for Fx22

No need. <input type=range> was disabled in v22.
Comment on attachment 722892 [details] [diff] [review]
Patch 1/2 - Layout prefs

My gut feeling is that these should be system metrics on the nsITheme implementation rather than prefs.

But maybe there's a reason that doesn't work?

(Sorry for taking so long to get to this.)
Attachment #722892 - Flags: review?(dbaron) → review-

Updated

4 years ago
Blocks: 879797

Updated

4 years ago
OS: Mac OS X → Android
Hardware: x86 → ARM
(Assignee)

Comment 31

4 years ago
Created attachment 763897 [details] [diff] [review]
Patch

Sorry this languished for awhile.

I've given up on themeing the checkboxes or radios in this bug then.

This instead just focuses on the inputs and button elements. I also dropped the padding on them by default, as some sites (I ran into Google Maps) depend on inputs having zero (or very little) padding. I also left styling range elements out. I think we need some UX love there before we move on.

I'll post a screenshot
Attachment #722892 - Attachment is obsolete: true
Attachment #728498 - Attachment is obsolete: true
Attachment #728498 - Flags: review?(mark.finkle)
Attachment #763897 - Flags: review?(mark.finkle)
(Assignee)

Comment 32

4 years ago
Created attachment 763899 [details]
Screenshot

Also, note the appearance of input[type="file"] changed recently. This is updated for that.
Attachment #728494 - Attachment is obsolete: true
Comment on attachment 763897 [details] [diff] [review]
Patch

I'm willing to give this a try. Let's land it soon, so we get decent coverage in Fx25.

I assume Ian is OK with the look and feel.
Attachment #763897 - Flags: review?(mark.finkle) → review+
Ian - Looking for a thumbs up/down from you
Flags: needinfo?(ibarlow)
(Reporter)

Comment 35

4 years ago
Let's land it. I have feedback but we can address it in follow up bugs, this is still a huge improvement over what we have.
Flags: needinfo?(ibarlow)
(Assignee)

Comment 36

4 years ago
I was about to push and thought there might be some reftests that don't like this. Turns out, I was right:

https://tbpl.mozilla.org/?tree=Try&rev=78ff3ba25b14

working through them...
(Assignee)

Updated

4 years ago
Duplicate of this bug: 900871
(Assignee)

Comment 38

4 years ago
Just to update. Some of these reftests look like:

<input type="text>
matches
<input type="text" style="padding: 0;">

With this patch, they don't. I'd love to just disable them, but I think webcontent probably relies on this behaviour. After talking to dbaron, he tells me there isn't a good standard for native theming in this sense. For instance, should:

<input type="text" style="padding: 10px;">

add padding on top of the native padding, or should it replace the native padding? The testcase above would indicate that Gecko wants to enforce the former.

I think maybe(?) if I implement some of this in nsINativeTheme we can work around these issues, so I've been looking at that, but it requires we do the drawing by hand in Thebes (i.e. setting any native theme properties overrides everything in content.css).
(Assignee)

Comment 39

4 years ago
In the mean time, we've gotten some complaints from frameworks that our current base theme is hard to deal with. Maybe we can implement something thats similar, but with less padding here.
(Assignee)

Comment 40

4 years ago
TBH, I don't have time to look at this.
Assignee: wjohnston → nobody

Updated

3 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → 34+
Duplicate of this bug: 1030581
Want to pick this up bug again. Where can I find a link to a comprehensive list of all the form elements I need to design for?
Flags: needinfo?(wjohnston)
(Assignee)

Comment 43

3 years ago
Created attachment 8450542 [details]
forms.html

Sure. Here's a page with all of them. Some are also listed here:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input?redirectlocale=en-US&redirectslug=HTML%2FElement%2FInput

This page doesn't quite have every disabled state in it, but most of them are the same as something else.

We CAN'T do whatever you dream of here very easily. i.e. We can basically only set borders and background-colors. Maybe border-radius. Anything else (images, padding, gradients, shadows, etc) is unlikely to be overridden by sites, and as such will likely break things.

We may be able to play with "inner" pieces of the widgets though. Things like the "Browse" button in File inputs, the little buttons to go up or down in number inputs. The arrow in <select> boxes. No custom radio or checkboxes though.

I'd like to do this in two phases if we can. One set of simple changes, and a larger bug for big changes (that will require writing some C++ code to draw widgets by hand).
Flags: needinfo?(wjohnston)
Created attachment 8452664 [details]
prev_forms_mock1.png

I gave this a quick whirl and wanted to post a WIP to see if this stuff was possible. I've kept it all pretty basic to say the least. Essentially I wanted to figure out a "quick win" approach and then go from there. 

Thoughts?
Flags: needinfo?(wjohnston)
(Reporter)

Comment 45

3 years ago
Thanks Anthony. I believe this bug is also about taking a look at things like basic text buttons, radios, checkboxes, sliders, and a few others. Check out Wes's test page of _all the things_ https://dl.dropboxusercontent.com/u/72157/forms.html
(Assignee)

Comment 46

3 years ago
Created attachment 8453313 [details] [diff] [review]
WIP v1

Short attempt at this. The mockups have more padding than we will probably be able to make. Moving the "Browse..." button on File inputs to the right didn't want to work for me. Not sure why, but it may just be impossible. We could probably localize it differently for mobile though. I'm not sure if we can replace it with an icon (for accessibility reasons). Will have to make sure that an aria label is set, even if there is not button text.

I can replace the <select> arrows with the up-down ones if you have graphics (SVG is what we use there so that these look nice scaled).

The magnifying glass for search boxes is probably something we shouldn't do either. No one else does by default, so we're likely to conflict with sites own styles.

Will play with ellipsizing and try to figure out why its not happening.

Build at:
http://people.mozilla.com/~wjohnston/forms.apk

Lots of polish left though. Always lots of polish left to do. And I just guessed on the widgets not in your picture.
Attachment #763897 - Attachment is obsolete: true
Attachment #763899 - Attachment is obsolete: true
Flags: needinfo?(wjohnston)
(Assignee)

Comment 47

3 years ago
Created attachment 8453316 [details]
Screenshot
(In reply to Wesley Johnston (:wesj) from comment #47)
> Created attachment 8453316 [details]
> Screenshot

Wow. This is mostly wonderful.

My only nit: The disabled "Browse..." button does not line up with the normal ones.

+1 on the SVG up/downs and not using the search image.
(Assignee)

Comment 49

3 years ago
Created attachment 8458335 [details] [diff] [review]
Patch v1

I think this feels pretty good. I made the svg myself. dholbert told me performance should be pretty good like this.

I also moved us to use css variables :) We don't have to do that..
Attachment #8453313 - Attachment is obsolete: true
Attachment #8458335 - Flags: review?(mark.finkle)
(Assignee)

Comment 50

3 years ago
Created attachment 8458336 [details]
forms.html

Updated with progress and meter elements. They aren't technically form elements, but they're good enough for me.
Attachment #8450542 - Attachment is obsolete: true
(Assignee)

Comment 51

3 years ago
Created attachment 8458337 [details]
Screenshot

And a pretty picture.
Attachment #8453316 - Attachment is obsolete: true
(Assignee)

Comment 52

3 years ago
Created attachment 8458340 [details] [diff] [review]
Patch

Staring at that screenshot, I noticed the weird brown in disabled radios and checkboxes, as well as on option elements. Updated to force them to use our normal colors.
Attachment #8458335 - Attachment is obsolete: true
Attachment #8458335 - Flags: review?(mark.finkle)
Attachment #8458340 - Flags: review?(mark.finkle)
Comment on attachment 8458340 [details] [diff] [review]
Patch


>diff --git a/mobile/android/themes/core/images/arrows.svg b/mobile/android/themes/core/images/arrows.svg

>+<!-- Created with Inkscape (http://www.inkscape.org/) -->

Needed?


>+    <style type="text/css">
>+.arrow {
>+  stroke-width: 1px;
>+  stroke-linecap: round;
>+  stroke-linejoin: round;
>+}
>+
>+.sprite {
>+  display: none;
>+  fill: black;
>+  stroke: black;
>+}
>+
>+.sprite:target {
>+  display: block;
>+}
>+
>+.disabled {
>+  fill: gray;
>+  stroke: gray;
>+}
>+    </style>

Does it matter if we indent the style section?

Let's wait to land this next week for Fx34 and give us time to look for problems. We can up lift if things look good.
Attachment #8458340 - Flags: review?(mark.finkle) → review+
(In reply to Wesley Johnston (:wesj) from comment #46)
> Created attachment 8453313 [details] [diff] [review]
> WIP v1
> 
> Short attempt at this. The mockups have more padding than we will probably
> be able to make. Moving the "Browse..." button on File inputs to the right
> didn't want to work for me. Not sure why, but it may just be impossible. We
> could probably localize it differently for mobile though. I'm not sure if we
> can replace it with an icon (for accessibility reasons). Will have to make
> sure that an aria label is set, even if there is not button text.

Hm, could you expand on the limitations we're working with here a little more? Maybe we could figure out other ways to give the form elements a little bit more breathing room. 

> I can replace the <select> arrows with the up-down ones if you have graphics
> (SVG is what we use there so that these look nice scaled).

I have an .svg file I could attach to this bug.

> The magnifying glass for search boxes is probably something we shouldn't do
> either. No one else does by default, so we're likely to conflict with sites
> own styles.

Indeed, I just kept those in there because I wanted to show how they'd line up to our current fields that I've been using mostly in my designs.

> Lots of polish left though. Always lots of polish left to do. And I just
> guessed on the widgets not in your picture.
Created attachment 8459249 [details]
mob_icon_select.zip

Here are .SVG versions of the select icon on dropdowns.

Let me know if these help!
Flags: needinfo?(wjohnston)
updates our Android 2.3 style form elements to a more modern look
relnote-firefox: --- → ?
Keywords: feature
(Assignee)

Comment 57

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/6acef5be504c
Flags: needinfo?(wjohnston)
(Assignee)

Comment 58

3 years ago
> Hm, could you expand on the limitations we're working with here a little
> more? Maybe we could figure out other ways to give the form elements a
> little bit more breathing room. 

I landed this, but we can continue to tweak. I suspect (but am not 100% sure) that adding horizontal padding will break some tests. We could disable those, but I've also seen a lot of sites that depend on inputs have 0 horizontal padding by default for their layouts. Adding some on can cause them to break badly. Vertical padding is less likely to cause problems.

> I have an .svg file I could attach to this bug.
Cool. The file I wound up using is... special :) in that I traced your original and then hand edited the svg into a set of sprites, all using one arrow, translated and scaled into different positions. I looked these over and I think we're pretty close, but if you want, I can incorporate these arrows in it instead.
sorry had to back this out for failing android reftests like https://tbpl.mozilla.org/php/getParsedLog.php?id=44490191&tree=Fx-Team
(Assignee)

Comment 60

3 years ago
Argh. Sorry. I pushed to try last week, but apparently didn't check the results.

Just to show how great these reftests are, we apparently have a test that:

<div style="padding: 0; margin: 0; border: none; background: transparent; -moz-appearance: none; width: 0; height: 0; font: inherit; padding: 1px 3px">
  Some text <div style="background: green; width: 100px; height: 100px"></div>
</div>

and 

<button style="padding: 0; margin: 0; border: none; background: transparent; -moz-appearance: none; width: 0; height: 0; font: inherit; color: black">
  Some text <div style="background: green; width: 100px; height: 100px"></div>
</button>

both render the same. i.e. a button has to be the same as a div with some random padding added to it. And that has to be true because webkit did that at some time in the past. Yay!

I'll dig through these and try to figure out what we need to make them pass... probably in a second patch. Alternatively, we could disable these tests on Android and turn them on one by one...
np :)

btw i noticed in the tbpl test results there is also https://tbpl.mozilla.org/php/getParsedLog.php?id=44488740&tree=Fx-Team

(just mentioning because this seems to be different failures than the others)

20:55:36     INFO -  2883 INFO TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug657143.html | SVG property list should be alphabetical - got "clip-path,clip-rule,color-interpolation,color-interpolation-filters,dominant-baseline,fill,fill-opacity,fill-rule,filter,flood-color,flood-opacity,image-rendering,lighting-color,marker-end,marker-mid,marker-start,mask,mask-type,paint-order,shape-rendering,stop-color,stop-opacity,stroke,stroke-dasharray,stroke-dashoffset,stroke-linecap,stroke-linejoin,stroke-miterlimit,stroke-opacity,stroke-width,text-anchor,text-rendering,vector-effect,moz-form-textcolor,moz-form-border-focused,moz-form-background-disabled,moz-form-border-invalid,moz-highlight-color,moz-form-button-background,moz-range-progress-disabled,moz-form-textcolor-placeholder,moz-form-border,moz-form-border-radius,moz-form-textcolor-disabled", expected "clip-path,clip-rule,color-interpolation,color-interpolation-filters,dominant-baseline,fill,fill-opacity,fill-rule,filter,flood-color,flood-opacity,image-rendering,lighting-color,marker-end,marker-mid,marker-start,mask,mask-type,moz-form-background-disabled,moz-form-border,moz-form-border-focused,moz-form-border-invalid,moz-form-border-radius,moz-form-button-background,moz-form-textcolor,moz-form-textcolor-disabled,moz-form-textcolor-placeholder,moz-highlight-color,moz-range-progress-disabled,paint-order,shape-rendering,stop-color,stop-opacity,stroke,stroke-dasharray,stroke-dashoffset,stroke-linecap,stroke-linejoin,stroke-miterlimit,stroke-opacity,stroke-width,text-anchor,text-rendering,vector-effect"
(In reply to Wesley Johnston (:wesj) from comment #58)
> I landed this, but we can continue to tweak. I suspect (but am not 100%
> sure) that adding horizontal padding will break some tests. We could disable
> those, but I've also seen a lot of sites that depend on inputs have 0
> horizontal padding by default for their layouts. Adding some on can cause
> them to break badly. Vertical padding is less likely to cause problems.

Good point. I'll keep this in mind as a I keep iterating on this.
Depends on: 1043461
(In reply to Carsten Book [:Tomcat] from comment #61)
> btw i noticed in the tbpl test results there is also
> https://tbpl.mozilla.org/php/getParsedLog.php?id=44488740&tree=Fx-Team
> 20:55:36     INFO -  2883 INFO TEST-UNEXPECTED-FAIL |
> /tests/layout/style/test/test_bug657143.html | SVG property list should be
> alphabetical

This looks to me like a bug in the test, apparently caused by adding CSS variables to a UA stylesheet. I filed bug 1043461 on it.
David, do you think it is OK that the CSS variables defined in the UA style sheet are exposed to authors?  In general, values set by the UA style sheet are visible to authors, but the use of variables seems like an implementation detail.  I'm not sure there's a good way to hide them, though.
Flags: needinfo?(dbaron)
It seems bad.  The question I'm not sure about is how bad.  I see two main problems at first glance:

 (1) It seems bad that we're exposing these particular CSS variables as an API to the Web for authors to use to change the styles of Fennec form elements, adding yet another set of unstandardized stuff to the story of form element styling.

 (2) It's not clear to me whether the form controls were designed under the threat model that these variables could be under hostile control; it seems like it might need security review.
Flags: needinfo?(dbaron)
Could we have the variables set on elements in the shadow trees rather than on the <input> etc. elements themselves?
(BTW, I talked about wesj about the variable-leakage (and the fact that authors can override the variable values) in MV today -- he's OK with removing the variables (via preprocessing) if they're problematic.  Preprocessing seems like a better solution here to me, unless/until we want to explicitly expose these colors as things that authors can use & override for site-theming purposes.)
(Assignee)

Comment 68

3 years ago
Created attachment 8468288 [details]
WIP Test fixes

Working my way through these. A small amount of them are actually tests that are fixed by the new styling. The new bustage mostly came from using an outline on invalid form field rather than box-shadow. I'm probably going to just switch back to box-shadow to avoid breaking sites.

A list of tests:
PASS | box-shadow/611574-1.html - removing background image makes this pass
PASS | box-shadow/611574-2.html - removing background image makes this pass
PASS | layout/reftests/bugs/424074-1.xul - scrollbars fixed this
FAIL | layout/reftests/bugs/491180-1 - inner focus ring spacing is required
FAIL | layout/reftests/bugs/491180-1 - inner focus ring spacing is required
PASS | layout/reftests/bugs/513318-2.xul - This test expects to fail because of the scrollbar. Old fennec hid them in xul. I removed that.
PASS | layout/reftests/bugs/560455-1.xul - Removing background image fixes it
FAIL | layout/reftests/bugs/966992-1.html - Our input[type=submit/reset/etc] padding rules are more specific than the tests input ones... I'm not sure how to fix this or why it didn't fail before.
PASS | layout/reftests/css-disabled/button/button-fieldset-2.html - :disabled vs [disabled] styling
PASS | layout/reftests/css-disabled/button/button-fieldset-3.html - :disabled vs [disabled] styling
PASS | layout/reftests/css-disabled/button/button-fieldset-legend-2.html - :disabled vs [disabled] styling
PASS | layout/reftests/css-disabled/button/button-fieldset-legend-3.html - :disabled vs [disabled] styling
FAIL | layout/reftests/css-disabled/select/select-fieldset-4.html - :disabled on select button, border shouldn't be there...
PASS | layout/reftests/css-enabled/button/button-fieldset-2.html - :disabled vs [disabled] styling
PASS | layout/reftests/css-enabled/button/button-fieldset-3.html - :disabled vs [disabled] styling
PASS | layout/reftests/css-enabled/button/button-fieldset-legend-2.html - :disabled vs [disabled] styling
PASS | layout/reftests/css-enabled/button/button-fieldset-legend-3.html - :disabled vs [disabled] styling
FAIL | layout/reftests/css-enabled/select/select-fieldset-4.html - :disabled on select button, border shouldn't be there...
FAIL | layout/reftests/css-required/css-required-text.html - test expects only a box-shadow change for invalid forms
FAIL | layout/reftests/css-required/css-required-file.html - test expects only a box-shadow change for invalid forms
FAIL | layout/reftests/css-required/css-required-password.html - test expects only a box-shadow change for invalid forms
FAIL | layout/reftests/css-required/css-required-tel.html - test expects only a box-shadow change for invalid forms
FAIL | layout/reftests/css-required/css-required-search.html - test expects only a box-shadow change for invalid forms
FAIL | layout/reftests/css-required/css-required-dyn-1.html - test expects only a box-shadow change for invalid forms
FAIL | layout/reftests/css-required/css-required-dyn-3.html - test expects only a box-shadow change for invalid forms
FAIL | layout/reftests/css-required/css-required-dyn-5.html - test expects only a box-shadow change for invalid forms
FAIL | layout/reftests/css-valid/input/input-customerror.html - test expects only a box-shadow change for invalid forms
PASS | layout/reftests/css-valid/input/input-disabled.html - Changed styling to make our rules less specific
PASS | layout/reftests/css-valid/input/input-dyn-disabled.html - Changed styling to make our rules less specific
FAIL | layout/reftests/css-valid/input/input-required-invalid.html - test expects only a box-shadow change
FAIL | layout/reftests/css-valid/input/input-email-invalid.html - test expects only a box-shadow change
FAIL | layout/reftests/css-valid/input/input-url-invalid.html - test expects only a box-shadow change
FAIL | layout/reftests/css-valid/input/input-pattern-invalid.html - test expects only a box-shadow change
FAIL | layout/reftests/css-valid/input/input-type-invalid.html - test expects only a box-shadow change
PASS | layout/reftests/css-valid/textarea/textarea-disabled.html - background-image fixed it
PASS | layout/reftests/css-valid/textarea/textarea-dyn-disabled.html - background-image fixed it

PASS | layout/reftests/forms/text-control-baseline-1.html - fix by removing background-image
FAIL | layout/reftests/forms/button/max-height.html - fixed by removing padding on buttons
FAIL | layout/reftests/forms/button/percent-height-child-1.html - fixed by removing forced padding
FAIL | layout/reftests/forms/button/percent-height-child-2.html - fixed by removing forced padding
FAIL | layout/reftests/forms/button/percent-width-child-1.html - fixed by removing forced padding
FAIL | layout/reftests/forms/button/percent-width-child-2.html - fixed by removing forced padding
FAIL | layout/reftests/forms/button/vertical-centering.html
FAIL | layout/reftests/forms/placeholder/placeholder-6-textarea.html

FAIL | layout/reftests/forms/input/number/number-same-as-text-unthemed.html
FAIL | layout/reftests/forms/input/number/number-similar-to-text-unthemed.html
FAIL | layout/reftests/forms/input/number/show-value.html
FAIL | layout/reftests/forms/input/number/number-disabled.html
FAIL | layout/reftests/forms/input/number/number-auto-width-1.html
FAIL | layout/reftests/forms/input/number/focus-handling.html
FAIL | layout/reftests/forms/input/number/number-selected.html
FAIL | layout/reftests/forms/input/range/direction-unthemed-1.html
PASS | layout/reftests/forms/input/range/moz-range-progress-1.html
FAIL | layout/reftests/forms/input/range/moz-range-progress-3.html
PASS | layout/reftests/forms/input/text/size-2.html
FAIL | layout/reftests/forms/meter/values.html
FAIL | layout/reftests/forms/meter/values-rtl.html
FAIL | layout/reftests/forms/meter/margin-padding.html
FAIL | layout/reftests/forms/meter/margin-padding-rtl.html
FAIL | layout/reftests/forms/meter/bar-pseudo-element.html
FAIL | layout/reftests/forms/meter/bar-pseudo-element-rtl.html
FAIL | layout/reftests/forms/meter/values-vertical.html
FAIL | layout/reftests/forms/meter/values-vertical-rtl.html
FAIL | layout/reftests/forms/meter/margin-padding-vertical.html
FAIL | layout/reftests/forms/meter/margin-padding-vertical-rtl.html
FAIL | layout/reftests/forms/meter/bar-pseudo-element-vertical.html
FAIL | layout/reftests/forms/meter/bar-pseudo-element-vertical-rtl.html
FAIL | layout/reftests/forms/meter/default-style/default-style.html
FAIL | layout/reftests/forms/meter/default-style/default-style-dyn.html
FAIL | layout/reftests/forms/progress/values.html
FAIL | layout/reftests/forms/progress/values-rtl.html
FAIL | layout/reftests/forms/progress/margin-padding.html
FAIL | layout/reftests/forms/progress/margin-padding-rtl.html
FAIL | layout/reftests/forms/progress/bar-pseudo-element.html
FAIL | layout/reftests/forms/progress/bar-pseudo-element-rtl.html
FAIL | layout/reftests/forms/progress/indeterminate-style-width.html
FAIL | layout/reftests/forms/progress/values-vertical.html
FAIL | layout/reftests/forms/progress/values-vertical-rtl.html
FAIL | layout/reftests/forms/progress/margin-padding-vertical.html
FAIL | layout/reftests/forms/progress/margin-padding-vertical-rtl.html
FAIL | layout/reftests/forms/progress/bar-pseudo-element-vertical.html
FAIL | layout/reftests/forms/progress/bar-pseudo-element-vertical-rtl.html
FAIL | layout/reftests/forms/progress/indeterminate-style-height.html
PASS | layout/reftests/forms/textarea/resize.html
PASS | layout/reftests/forms/textarea/ltr.html
PASS | layout/reftests/forms/textarea/ltr.html
PASS | layout/reftests/forms/textarea/rtl.html
PASS | layout/reftests/forms/textbox/setsize.xul

PASS | layout/reftests/native-theme/native-theme-disabled-cascade-levels.html
FAIL | layout/reftests/svg/foreignObject-form-theme.svg

FAIL | widget/reftests/progressbar-fallback-default-style.html
FAIL | widget/reftests/meter-fallback-default-style.html
FAIL | layout/xul/grid/reftests/scrollable-columns.xul
FAIL | content/html/content/reftests/autofocus/input-number.html

FAIL | layout/style/test/test_value_storage.html

Updated

3 years ago
Duplicate of this bug: 1049690

Updated

3 years ago
tracking-fennec: 34+ → ?
(Assignee)

Comment 70

3 years ago
Trying again:
https://tbpl.mozilla.org/?tree=Try&rev=d63b59bb3638

This is pretty drastically pulled back. Just removes gradients and replaces them with colors from this bug. And updates borders and border-radius values. I expect to see some tests start to pass and show up as failures.
(Assignee)

Comment 71

3 years ago
All of the failures in that push were tests that started passing. Fixed and repushed:
https://tbpl.mozilla.org/?tree=Try&rev=67c639e5a958
(Assignee)

Comment 72

3 years ago
Created attachment 8481539 [details] [diff] [review]
Patch

Looks good on try!
Attachment #8458337 - Attachment is obsolete: true
Attachment #8458340 - Attachment is obsolete: true
Attachment #8459249 - Attachment is obsolete: true
Attachment #8468288 - Attachment is obsolete: true
Attachment #8481539 - Flags: review?(mark.finkle)
Comment on attachment 8481539 [details] [diff] [review]
Patch

I think it's best to wait for Fx35 and then uplift if we want to and if the risk is low enough.
Attachment #8481539 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 74

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/b5a85757ac0e
https://hg.mozilla.org/mozilla-central/rev/b5a85757ac0e
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
tracking-fennec: ? → 34+
(Assignee)

Comment 76

3 years ago
I scoped this back to get it landed. This just removed the gradient (even from radio buttons and checkboxes), and updated the border-color/radius and text colors.

I'll file separate bugs for other things I'd like to do.
Summary: New designs for fennec form elements → Remove gradients from form elements
(Assignee)

Updated

3 years ago
Blocks: 1062978
(Assignee)

Updated

3 years ago
Blocks: 1062980
(Assignee)

Comment 77

3 years ago
I scoped this back to get it landed. This just removed the gradient (even from radio buttons and checkboxes), and updated the border-color/radius and text colors.

I'll file separate bugs for other things I'd like to do.
(Assignee)

Updated

3 years ago
Blocks: 1062985
(Assignee)

Updated

3 years ago
Blocks: 1062987
(Assignee)

Updated

3 years ago
Depends on: 959754
(Assignee)

Comment 78

3 years ago
Comment on attachment 8481539 [details] [diff] [review]
Patch

I'm going to nom this to go all the way to beta. We've hated these forever. Its just taken awhile to fix them. They're a big thorn in our mobile web compat side.

Approval Request Comment
[Feature/regressing bug #]: forever
[User impact if declined]: ugly forms
[Describe test coverage new/current, TBPL]: this actually made a lot of tests pass on tbpl :) there are tons of reftets for these things.
[Risks and why]: low risk. affects default form display of websites, but should bring it back in line with desktop/other browsers.
[String/UUID change made/needed]: none.
Attachment #8481539 - Flags: approval-mozilla-beta?
Attachment #8481539 - Flags: approval-mozilla-aurora?
(In reply to Wesley Johnston (:wesj) from comment #78)
> I'm going to nom this to go all the way to beta. We've hated these forever.
> Its just taken awhile to fix them. They're a big thorn in our mobile web
> compat side.

Can you expand on how this impacts web compat?
Flags: needinfo?(wjohnston)
(Assignee)

Comment 80

3 years ago
AFAIK, we're the only mobile browser that has a "default" background-image on form elements. Since no one else does, sites often don't add background-image: none; when styling form elements resulting in a different appearance in Firefox.

This removes that background-image entirely and brings us more in line with other browsers/webdev expectations for default styles.
Flags: needinfo?(wjohnston)
The main thing this fixes is that sites that use a button for their main navigation no longer have a gradient. Compare a medium.com [1] article in release and nightly for example. This affects the M in the upper right of the page because they use a button element for the navigation.

[1] https://medium.com/stuff-and-more-stuff/making-a-unicorn-tear-dc25dd9a1c79
Comment on attachment 8481539 [details] [diff] [review]
Patch

Thanks Wes and Kevin. That is pretty visible. As the patch looks safe, let's take this for mobile beta4. Beta+ and Aurora+
Attachment #8481539 - Flags: approval-mozilla-beta?
Attachment #8481539 - Flags: approval-mozilla-beta+
Attachment #8481539 - Flags: approval-mozilla-aurora?
Attachment #8481539 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf27552884dd
https://hg.mozilla.org/releases/mozilla-beta/rev/7b4d4b3b7598
status-firefox33: --- → fixed
status-firefox34: --- → fixed
status-firefox35: --- → fixed
Added to the release notes with "Form elements updated to a more modern look" as wording.
Don't hesitate if you have a better wording to suggest!
relnote-firefox: ? → 33+
You need to log in before you can comment on or make changes to this bug.