763671 - Remove gradients from form elements

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

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.

Comment 1

[Ian Barlow (:ibarlow)]

New designs! http://cl.ly/image/441g2z2X3p0r

Comment 2

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Screenshot old vs new

Build is at:

http://dl.dropbox.com/u/72157/fennec-forms.apk

Comment 5

[Wesley Johnston (:wesj)]

My test page is at http://dl.dropbox.com/u/72157/forms.html

Comment 6

[Ian Barlow (:ibarlow)]

Omg. 

This looks excellent. Let's ship it! \o/

Comment 7

[Ian Barlow (:ibarlow)]

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 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 9

[Wesley Johnston (:wesj)]

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....

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Revised disabled elements

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

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

grrr. gradients.

Comment 12

[Ian Barlow (:ibarlow)]

(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 :)

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 14

[Wesley Johnston (:wesj)]

Attachment: 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).

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Layout prefs

This adds the prefs I needed. If you know of a better solution, happy to hear!

Comment 16

[Wesley Johnston (:wesj)]

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

Dholbert, if you're comfortable reviewing thins, feel free to steal.

Comment 17

[Wesley Johnston (:wesj)]

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.

Comment 18

[Ian Barlow (:ibarlow)]

(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.

Comment 19

[Daniel Holbert [:dholbert]]

(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.)

Comment 20

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Oh yeah, I restyled the select elements to be a bit more androidy too (without going to far overboard.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Updated patch again

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Also, given this is the last week of the Fx22 cycle, I think we should land these in Fx23.

Comment 25

[Wesley Johnston (:wesj)]

(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.

Comment 26

[Jonathan Watt [:jwatt]]

(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.

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 29

[Jonathan Watt [:jwatt]]

(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 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 31

[Wesley Johnston (:wesj)]

Attachment: 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

Comment 32

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Also, note the appearance of input[type="file"] changed recently. This is updated for that.

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

Ian - Looking for a thumbs up/down from you

Comment 35

[Ian Barlow (:ibarlow)]

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.

Comment 36

[Wesley Johnston (:wesj)]

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...

Comment 38

[Wesley Johnston (:wesj)]

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).

Comment 39

[Wesley Johnston (:wesj)]

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.

Comment 40

[Wesley Johnston (:wesj)]

TBH, I don't have time to look at this.

Comment 42

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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?

Comment 43

[Wesley Johnston (:wesj)]

Attachment: 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).

Comment 44

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: 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?

Comment 45

[Ian Barlow (:ibarlow)]

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

Comment 46

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 47

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 48

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 49

[Wesley Johnston (:wesj)]

Attachment: 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..

Comment 50

[Wesley Johnston (:wesj)]

Attachment: forms.html

Updated with progress and meter elements. They aren't technically form elements, but they're good enough for me.

Comment 51

[Wesley Johnston (:wesj)]

Attachment: Screenshot

And a pretty picture.

Comment 52

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 53

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 54

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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.

Comment 55

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: mob_icon_select.zip

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

Let me know if these help!

Comment 56

[Kevin Brosnan [Ex-Mozilla]]

updates our Android 2.3 style form elements to a more modern look

Comment 57

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/6acef5be504c

Comment 58

[Wesley Johnston (:wesj)]

> 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.

Comment 59

[Carsten Book [:Tomcat]]

sorry had to back this out for failing android reftests like https://tbpl.mozilla.org/php/getParsedLog.php?id=44490191&tree=Fx-Team

Comment 60

[Wesley Johnston (:wesj)]

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...

Comment 61

[Carsten Book [:Tomcat]]

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"

Comment 62

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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.

Comment 63

[Daniel Holbert [:dholbert]]

(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.

Comment 64

[Cameron McCormack (:heycam)]

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.

Comment 65

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 66

[Cameron McCormack (:heycam)]

Could we have the variables set on elements in the shadow trees rather than on the <input> etc. elements themselves?

Comment 67

[Daniel Holbert [:dholbert]]

(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.)

Comment 68

[Wesley Johnston (:wesj)]

Attachment: 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

Comment 70

[Wesley Johnston (:wesj)]

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.

Comment 71

[Wesley Johnston (:wesj)]

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

Comment 72

[Wesley Johnston (:wesj)]

Attachment: Patch

Looks good on try!

Comment 73

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 74

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/b5a85757ac0e

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5a85757ac0e

Comment 76

[Wesley Johnston (:wesj)]

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.

Comment 77

[Wesley Johnston (:wesj)]

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.

Comment 78

[Wesley Johnston (:wesj)]

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.

Comment 79

[Lawrence Mandel [:lmandel] (use needinfo)]

(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?

Comment 80

[Wesley Johnston (:wesj)]

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.

Comment 81

[Kevin Brosnan [Ex-Mozilla]]

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 82

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf27552884dd
https://hg.mozilla.org/releases/mozilla-beta/rev/7b4d4b3b7598

Comment 84

[Sylvestre Ledru [:Sylvestre]]

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!