woocommerce / storefront

Official theme for WooCommerce
https://wordpress.org/themes/storefront/
972 stars 471 forks source link

Required checkout fields showing a dotted underline under the required indicator #1262

Open mikkamp opened 4 years ago

mikkamp commented 4 years ago

Describe the bug

When viewing the checkout page, the required fields show a red dotted underline under the * character:

image

The element which is used for this text is <abbr>, which some browser style by default with a dotted underline. See: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/abbr

Isolating the problem (mark completed items with an [x]):

To Reproduce

Steps to reproduce the behavior:

  1. Go to https://themes.woocommerce.com/storefront/
  2. Add a product to the cart and proceed to the checkout page
  3. Notice the extra dotted underline on the required fields

Expected behavior

I'd expect this style to be reset so it doesn't show an underline. The CSS for this element is already being altered here: https://github.com/woocommerce/storefront/blob/version/2.5.4/assets/css/woocommerce/woocommerce.scss#L1572-L1575 So it could get an extra line to reset the text-decoration

Browser Environment

It will depend on the Browser which is being used, in my case I'm using Chrome on Ubuntu.

WordPress Environment

Please provide relevant details of your WordPress setup and server environment.

``` WordPress 5.3.2 WooCommerce 3.9.2 Storefront 2.5.4 ```
haszari commented 4 years ago

Thanks @mikkamp - reproduced this in 2.5.4 and 2.5.3.

Looks like a simple fix so I've added this to 2.5.5 milestone.

I checked Twenty Twenty and Twenty Nineteen, and both reset abbr to prevent the dotted underline, so I think it's worth doing the same in Storefront.

I wonder if we should consider fixing this upstream in WooCommerce core? cc @peterfabian

haszari commented 4 years ago

I checked Twenty Twenty and Twenty Nineteen, and both reset abbr to prevent the dotted underline, so I think it's worth doing the same in Storefront.

Update – I was mistaken, these theme fixes are in WooCommerce core. I'm reporting this issue there, so hopefully it can be fixed for all themes.

I've opened a PR with the fix in Storefront as a fallback – woocommerce/woocommerce#1264.

haszari commented 4 years ago

This is now logged to WooCommerce core repo: https://github.com/woocommerce/storefront/issues/2156

haszari commented 4 years ago

Removing this from 2.5.5 – it's not critical or urgent, and wasn't introduced in Storefront 2.5.3.

nielslange commented 4 years ago

@haszari As this issue is related to the WooCommerce core rather than the Storefront theme, can this ticket be closed?

haszari commented 4 years ago

Closing - to follow / fix this issue please visit woocommerce/storefront#2156

nielslange commented 4 years ago

Reopening this issue, as this issue it not caused by WooCommerce but by Storefront. WooCommerce already uses text-decoration: none; for this element in https://github.com/woocommerce/woocommerce/blob/f25967d57bc50c2906c94c111966d1bdbac04d1f/assets/css/woocommerce.scss#L1361-L1367. However, Storefront dequeues the WooCommerce styles in https://github.com/woocommerce/storefront/blob/b88139bbf4daecf7cbb36005ba5ebc30586c3da4/inc/woocommerce/hooks.php#L13 Hence, the fix should be applied within Storefront.

@haszari & @juliaamosova Any thoughts on this or shall I go ahead and close https://github.com/woocommerce/storefront/issues/2156 in benefit of this ticket?

haszari commented 4 years ago

Good sleuthing @nielslange ! That's surprising, feels like Woo core and Storefront are working at cross purposes here.

I think this means that this issue could be fixed in either place - and I'm not sure what the best solution is. I'm happy to keep both issues open for now.

nielslange commented 4 years ago

I think this means that this issue could be fixed in either place - and I'm not sure what the best solution is. I'm happy to keep both issues open for now.

Not really. Technically, the issue is already fixed within WooCommerce and Storefront simply does not show that fix due to https://github.com/woocommerce/storefront/blob/b88139bbf4daecf7cbb36005ba5ebc30586c3da4/inc/woocommerce/hooks.php#L13

Unless we open up Storefont in a way that the WooCommerce styles become active again, this issue needs to be addressed within Storefront. You know what I mean, @haszari? 🙂

haszari commented 4 years ago

Unless we open up Storefont in a way that the WooCommerce styles become active again, this issue needs to be addressed within Storefront.

Agreed, just feels unfortunate that we would reimplement a style rule which we get for free but then we've disabled (de-enqueued).

Re-enqueuing the Woo styles (removing the override) is an option, though I'm assuming there's a reason we're doing this and to change now would break other things. (Which is also unfortunate!)

Woo also has a storefront-specific stylesheet where we could presumably fix this, and there are probably other approaches we could take :)