Remove styling of select elements (improves appearance in Firefox)

I have fixed this issue in my local, I am attaching patch file.

Updated patch for the issue.

Check my screen below about selectboxes in FF for Bartik theme. This is how it looks for now on 8.1.x. Not sure about how it should looks?

Selectboxes in my ubuntu FF v44.0.2

Was noticed about bad inner borders for some fields. This patch will fix this issue
After my changes for #edit-name and #edit-mail fields related to inner borders

@ironkiat
I have uploaded the correct path in #6 which is working fine.

@kostyashupenko
if you are uploading a new patch, your patch number should be with respect to your comment number. may i know the reason why you deleted drupal-select-box-un-styled-ff-2674292-6.patch file from the files list.

There's a misunderstanding here: kostyashupenko did not delete your patch just hidden it -- it's customary to only show the last patch under the issue summary to make it easier to review. If you click the relevant fieldset in #7 you will see that your patch is still there, together with its test result. They couldn't delete anyways and if you check the "git command" box below you see you will be credited when this patch gets in.

On the other hand, @kostyashupenko please post a review next time and respect the issue being assigned to tshivasrikanth . Yes sometimes taking over an issue is necessary -- either the assigned just wandered off and there's nothing happening any more or the patch is so broken that a review would be longer than fixing it. Neither of these are the case here. Please let tshivasrikanth finish this issue.

Updated patch for the branch 8.2.x-dev

I don't see the harm in including this in 8.1.x so putting back there at least tentatively. For what it's worth the patch in #14 at least applies to 8.1.x.

Chrome is a Winner in Drupal!! haha. Reminded me the awful looks in IE.

+++ b/core/themes/bartik/css/components/form.css
@@ -100,8 +100,7 @@ input.form-search,
   color: #3b3b3b;

still same color, right ?

I'm setting this as a 'Feature Request' as there are currently intentional styles set in Bartik for form elements, nothing is actually "unstyled" they are actually overridden.
I was not part of that decision making, but as maintainer I'm going to go poke into this reasoning further.

The form elements are designed to be grey in Bartik so the grey select styling needs to stay.

However(!)....The exact same scenario came up in the Seven theme. The only browser that used the CSS only select styling correctly were Webkit ones, so we dropped styling support for every other browser and made the styles Webkit specific.

So the plan forward is that we make the select.form-select styles Webkit only styling which will allow Firefox to look less dreadful.

Here is the Seven issue for reference... #2207391: Style select elements in Webkit only.

Hey I just took patch #14 and added the same appraoch taken in #2207391: Style select elements in Webkit only.

It's making the webkit a bit more look like we want it and keeps other browsers untouched.

@emma.maria ist that what we were looking for?

Hi @alamowo. I haven't looked at the patch test but we want to set the styles we currently have for select elements in Bartik right now in core, to be set for only WebKit browsers.

Thanks @alamowo for the patch!

Bartik now assigns select styling only to Webkit browsers
 

 

 
and
Firefox and other non-Webkit browsers now use the default browser select styling...
 

 

 

However I noticed one small thing that you have missed....

+++ b/core/themes/bartik/css/components/form.css
@@ -92,6 +92,11 @@ textarea.form-textarea,
+@media screen and (-webkit-min-device-pixel-ratio: 0) {
+  select.form-select {
+    border: 1px solid #ccc;
+  }
+}
 input.form-text,
 input.form-tel,
 input.form-email,
@@ -100,8 +105,7 @@ input.form-search,

@@ -100,8 +105,7 @@ input.form-search,
 input.form-file,
 input.form-number,
 input.form-color,
-textarea.form-textarea,
-select.form-select {
+textarea.form-textarea {
   border: 1px solid #ccc;
   color: #3b3b3b;
 }

Can you add the color: #3b3b3b; style also to the Webkit select element please? Thanks!

Added the color: #3b3b3b; style to the Webkit select element.

@sskt Due to an error "500" in version "8.2.x" I can not review it. As I can do.
---> composer install

i'm reviewing :)

After applying the patch, issue #29. In the select Chrome have a pixel height less than the inputs.

But in Chrome (linux) works fine

In Firefox (osx) and Firefox (linux) looks small.

For Bartik Theme is the same.

change status -> Needs Work

Add patch to fix Chrome osx Select Box.

@emma.maria please i would like to talk with you for this issue.

This could really use Emma's eyes

1. +++ b/core/themes/bartik/css/components/form.css
   @@ -92,6 +92,14 @@ textarea.form-textarea,
   +    height: 27px;
   

   Why 27px specifically? It's not used anywhere else in Bartik

2. +++ b/core/themes/bartik/css/components/form.css
   @@ -92,6 +92,14 @@ textarea.form-textarea,
   +    margin-top: 1px;
   

   Not sure why this 1px is needed, probably should have a comment explanation.

please review my patch is it working fine. I 've tested and also attach screen shot

@Mukesh: Changes that you have done in last patch are working as per the expectations. Please check the screenshots that i have captured after applying the patch.

But one issue that i have noticed that the height of selectboxes in chrome (Linux). And one other issue is in FF, The alignment of textboxes and selectboxes is not correct. Please check screenshots.

checking this thing

can't reproduce these screens

Because Webkit & Mozilla Firefox rendering engines implement line height differently - we get different results as seen in the screenshots.

We have to explicitly set the line-height and use padding to make them behave the same way.

#57 patch with DCS fix and Screenshots.

Re-adding patch #50

+++ b/core/themes/bartik/css/components/form.css
@@ -112,8 +111,8 @@ input.form-search,
+ {

This should be on line above.

Oh, sorry, that's terrible feedback. There's a curly brace that should be at the end of the line preceding it.

Thanks for the patch.

+++ b/core/themes/seven/css/components/form.css
@@ -189,6 +188,12 @@ textarea.form-textarea {
+@media screen and (-webkit-min-device-pixel-ratio:0) {

Sorry, think there should be a space after the colon here too.

DCS fixes added.

I tested this in Firefox 53 and the changes look good to me. Screenshot attached.

+++ b/core/themes/bartik/css/components/form.css
@@ -112,8 +111,8 @@ input.form-search,
+textarea.form-textarea
+ {

{ should be on same line

Looks like #59 has been addressed in the patch in #57.

@tim.clifford thanks for the updated patch! Providing an interdiff is very useful to reviewers.

+++ b/core/themes/seven/css/components/form.css
@@ -189,6 +188,12 @@ textarea.form-textarea {
+@media screen and (-webkit-min-device-pixel-ratio: 0) {

This is WebKit-specific. Let's also support other browsers.

A quick web search led me to https://www.w3.org/blog/CSS/2012/06/14/unprefix-webkit-device-pixel-ratio/.

Oh… apparently we want to do this in a WebKit-specific way! Fine, but then we need to document it as such.

Okay, so...I'm trying to help @starshaped get this patch done, but something is weirding me out.

As far as I can tell, the line-height fix in #50 doesn't seem to have to anything to do with the original issue, which is fixed by the patch in #48. I'm not saying it's something that shouldn't be fixed, just that it seems to be out-of-scope.

It seems to me that #48 is good to go as-is, and we should open a follow-up issue to fix the line-height thing. So I'm marking this as needing review for #48, and needing a follow-up.

As per phenaproxima's comment in #62, I re-rolled this without the line height changes. Screenshot attached from Firefox 53.

Hey all, had a look at the comments and latest patch.

Just confirming that the patch fixes the issue in firefox with the spacing. Did regression testing and couldn't find any problems with the patch (small changes as is).

Before:

After:

In Scope:

The line-height fix should be included as it sets the standard across the different rendering engines.

Out of Initial Scope:

-webkit-device-pixel-ratio is a non-standard boolean CSS media feature which is an alternative to the standard resolution media feature.

More information can be found here; https://developer.mozilla.org/en-US/docs/Web/CSS/@media/-webkit-device-p...

Okay, then let us re-roll the patch with the line-height fix. Thanks for the review, @AaronChristian!

Re-rolled as per AaronChristian's comment. Attached are 3 screenshots from Firefox 53, one of the fixed select in Bartik, the other two of the input text field and textarea with the fixed line-height in Seven.

Looks great @starshaped!

Thanks for that small addition & all the screenshots.

Unless opposed I'd like to mark this as RTBC.

If it's been reviewed and tested by a community member who's not the patch author, and it looks good, it's RTBC by definition :) Thanks, @AaronChristian and @starshaped!

Yep, looks good. Thanks starshaped and tim.clifford.

Great work with the screenshots and timely responses.

Seconding the RTBC.

Thanks all, this is a nice improvement. I double checked on simplytest and yes, this does make the select list look a lot better in Firefox.

I've updated the commit credits, this is ready to commit.

I don't follow why this needs to update Seven's CSS. I don't understand the argument made in #64. Other than that one change to Seven, this patch looks ready to go. So I would tend to agree with @phenaproxima's proposed direction in #62, let's get the straightforward change in and discuss the other issue separately.

Thanks everyone for the efforts here!

Finally went ahead and removed the line-height update as suggested in #71. Now this patch should be ready to go! :)

That looks RTBC, not touching Seven, only bartik.

Committed 24959f6 and pushed to 8.5.x. Thanks!