Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elements: Add support for text based inputs #51337

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented Jun 8, 2023

What?

Adds support for textarea and text-based inputs elements to theme.json.

Why?

Many themes want to control the display of textareas and other inputs, and this will allow them to do it: eg. https://github.com/WordPress/community-themes/pull/41/files

How?

Adds textInput to the valid elements, with a selector to target textarea and other text based input elements. The other changes are whitespace. We'll have to followup with a PR to add this to the UI later.

Testing Instructions

This can be tested in the bluenote theme, using this PR: https://github.com/WordPress/community-themes/pull/41/files. You will need to add a post comment form block to a template to see it.

Screenshots or screencast

Screenshot 2023-06-08 at 15 26 45

@scruffian
Copy link
Contributor Author

Also cc @aristath as he's been looking at form things.

@scruffian scruffian changed the title Elements: Add support for text area Elements: Add support for text based inputs Jun 8, 2023
@github-actions
Copy link

github-actions bot commented Jun 8, 2023

Flaky tests detected in 3ca066d.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5257316933
📝 Reported issues:

@carolinan
Copy link
Contributor

carolinan commented Jun 13, 2023

Do we need to exclude more items? The input type submit is excluded, but file and reset are not.
What about range? Basic settings like background color overlaps, but one might expect that if range is supported, styling the track and thumb should work too. It is easiest to exclude it.

@MaggieCabrera
Copy link
Contributor

Do we need to exclude more items? The input type submit is excluded, but file and reset are not. What about range? Basic settings like background color overlaps, but one might expect that if range is supported, styling the track and thumb should work too. It is easiest to exclude it.

You are right. We were discussing if it's longer to include or exclude. There is a longer list of them than I thought at the top of my head: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#input_types

@scruffian
Copy link
Contributor Author

scruffian commented Jun 13, 2023

I think a list of included or excluded types is going to be long. My preference is probably just to simplify the selector to all inputs, except buttons.

Actually on reflection I don't agree with this, I think an allow list is best, I'll update the PR.

@scruffian
Copy link
Contributor Author

Updated to use this selector:

textarea, input:where([type=email],[type=number],[type=password],[type=search],[type=tel],[type=text],[type=tel],[type=url])

Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the testing instructions and I see the expected styles in the editor. But in the front-end, the default styles end up overriding the styles I've set in elements.textInput.

Would it make sense to use the class selectors to target those blocks? I made a suggestion below.

'cite' => 'cite',
'caption' => '.wp-element-caption, .wp-block-audio figcaption, .wp-block-embed figcaption, .wp-block-gallery figcaption, .wp-block-image figcaption, .wp-block-table figcaption, .wp-block-video figcaption',
'cite' => 'cite',
'textInput' => 'textarea, input:where([type=email],[type=number],[type=password],[type=search],[type=tel],[type=text],[type=tel],[type=url])',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'textInput' => 'textarea, input:where([type=email],[type=number],[type=password],[type=search],[type=tel],[type=text],[type=tel],[type=url])',
'textInput' => 'textarea, input:where([type=email],[type=number],[type=password],[type=search],[type=tel],[type=text],[type=tel],[type=url]), .wp-block-search__input, .wp-block-post-comments-form textarea',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look. I don't think this is the right way round. Elements are meant to be a low level API so their CSS should be as non-specific as possible. If blocks chose to override these elements then that is their decision. One option might also be to add a selector like wp-element-text-input, so that blocks can decide to apply these rules to other elements if they want to.

@MaggieCabrera
Copy link
Contributor

Now that I'm thinking about this, I remember having the same discussion when we did this for the button element. We may not want to make that decision for every instance of text fields ever, but just core blocks while allowing third parties of any kind to extend that. I think we should target a class instead of the HTML elements. We do the same for figcaption too, and probably should have done the same for cite, but that's less common, and maybe that's why there haven't been complains.

@ghost
Copy link

ghost commented Jul 10, 2023

Hello. I studied this issue and the proposed solution.

  1. The proposed solution is not being applied to: core-loginout block inputs.
  2. [type="tel"] is written twice in the proposed solution.
  3. There is need to stylize input[type="checkbox"], since it is used by WordPress below the comment form (checkbox: “Save my name, email, and website in this browser for the next time I comment.”).
  4. In the proposed solution, you added support for: type=number, and type=tel. As long as I know, these input types are not used by WordPress in the front-end. If you are going to add support for random types, then you could very well also support: input[type="date"], since this one is commonly used.
  5. The proposed solution is not working in all scenarios.
    5.1. For example: when we add :hover and :focus pseudo-classes, there are all sorts of conflicts with the blocks stylesheets.
    5.2. Another example: the proposed solution targets the Comments Form Block in this part:
input:where([type=email],[type=number],[type=password],[type=search],[type=tel],[type=text],[type=tel],[type=url]), ... {

}

And currently the Comments Form Block contains the following stylesheet:

.wp-block-post-comments-form input:not([type="submit"]):not([type="checkbox"]), .wp-block-post-comments-form textarea](input:where([type=email],[type=number],[type=password],[type=search],[type=tel],[type=text],[type=tel],[type=url])) {
	padding: calc(.667em + 2px);
}
.wp-block-post-comments-form input:not([type="submit"]), .wp-block-post-comments-form textarea {
	border: 1px solid #949494;
	font-family: inherit;
	font-size: 1em;
}

Currently, the proposed solution is not overriding the current stylesheet associated to this block.

The proposed solution could contain at least the following, in order to function accordingly to expected:

.wp-block-post-comments-form input:not([type=submit]):not([type="checkbox"]) {

}

In WordPress 6.2, one proposal that is going to correctly satisfy all the needs of (A) Input and Textarea tags + (B) Core blocks (Login/out + Search + Comments form), including their -> hover and focus states <-, is the following:

input:where([type="email"],input[type="text"],[type="password"],[type="url"]), textarea,
.wp-block-post-comments-form input:not([type=submit]):not([type="checkbox"]), .wp-block-post-comments-form textarea,
.wp-block-search .wp-block-search__input {
	
}

This is going to continue to work when it comes the time of adding hover and focus states.

  1. Once all the scenarios have been explored, and the current proposal has been improved, it becomes evident that there is a need for, at least, one class, to exist and be applied to inputs. As suggested before, it could be called:
    .wp-element-input

However I would like to note, that the list of inputs is large enough and their applications is very distinct from each other. Therefore it is subjective what inputs we need or doesn't need to consider. I do not not like, for instance, the current proposal, which opts out for some inputs in favor of others... Specially when you think of plugins. I don't know how the current proposal or ".wp-element-input" are going to satisfy WooCommerce, for example. Which is going to bring "range", "radio" inputs.

In addition to:
(1.) fixing the current proposal;
(2.) introducing a ".wp-element-input";
(3.) and reviewing all blocks stylesheets once this pull request is merged;
I would like to suggest that in a fourth moment (a subsequent pull request)
(4.) a person should be able to extend in Theme.json what types of inputs the theme is targeting with the class .wp-element-input.
4.1. For example:
Write the following in Theme.json.

settings:
	inputs:
		types: "text, url, tel, range, checkbox"
	}
}

In this scenario, .wp-element-input is going to become: .wp-element-input:where([type="text"],input[type="url"],[type="tel"],[type="range"],[type="checkbox"])

Or maybe you are going to think of more extensible (the perfect scenario...) ways of targeting inputs with Theme.json, such as:

settings:
	inputs:
		classes:
			"text": ".wp-element-input-1",
			"url": ".wp-element-input-1",
			"email": ".wp-element-input-1",
			"number": ".wp-element-input-1",
			"range": ".wp-element-input-2",
			"radio": ".wp-element-input-2",
			"checkbox": ".wp-element-input-2",
		}
	}
}

This suggestion would better improve the situation for designers and plugins, while keeping it simple for casual users. A default fallback would happen when nothing is customized in Theme.json. And this whole specificity conflict will never exist again, as more blocks are introduced.

I hope to help with my feedback.

@scruffian
Copy link
Contributor Author

@meerhimmel thanks for the feedback. What if each type of input had its own element, so we'd have this:

			"text": ".wp-element-text",
			"url": ".wp-element-url",
			"email": ".wp-element-email"
			"number": ".wp-element-number",
			"range": ".wp-element-range",
			"radio": ".wp-element-radio",
			"checkbox": ".wp-element-checkbox",

@ghost
Copy link

ghost commented Jul 11, 2023

@scruffian Your proposed solution may be the most solid, ...as long as all 22 input types are going to have a correspondent class. (Just a few input types are styled by default, but one person or team needs to be able to customize each input if needed in theme.json).

My thoughts:
(1) In your proposal, theme authors and plugin developers are going to be able to target precisely the inputs, without the need to think about potential specificity conflict.

If instead of all input types being considered in the implementation, only some inputs are given a corresponding class, we would incur in the scenario where, in order to target the leftovers, I am afraid each plugin developer and theme author would apply a solution with different specificity, (as it currently happens), which is a nightmare to solve when multiple stylesheets are combined. Not elegant.

(2) The work being done by @aristath in the Form Block is also pointing in the direction of modularity as I understand: #44214 It is going to exist a Core Form Block which contains the most common input types available by default. I understand that plugins are going to expand the Form Block with more innerblocks equivalent to more input types as needed. So it would be awesome if all those plugins could target the same classes as Core does, as it happens with other elements...

(3) I also suspect that your proposal @scruffian is going to enable the style engine the needed flexibility to output styles in simpler ways. If it happens, a deprecation of input styles may not be hard to outcome in this scenario.

@mtias mtias mentioned this pull request Jul 14, 2023
17 tasks
@bph bph added the [Type] Enhancement A suggestion for improvement. label Sep 28, 2023
@fabiankaegy
Copy link
Member

This ticket is still listed on the WordPress 6.5 editor tickets board. Though it hasn't seen any movement in a while 🤔

We are 1 week out from Beta 1. Is this still something we want to / can get in before then?

Whats currently blocking this PR?

@scruffian
Copy link
Contributor Author

Whats currently blocking this PR?

Lack of agreement about whether this is the right approach. I suggest we take it off the 6.5 board.

@carolinan
Copy link
Contributor

What is needed to solve this for 6.7?

@carolinan
Copy link
Contributor

Also is there an open issue for this that should be linked to the PR?
Besides the text input, there are core blocks that uses a select element on the front, which needs styling.

@scruffian
Copy link
Contributor Author

What is needed to solve this for 6.7?

Some agreement about the selectors to use. Should we target all inputs, or only specific types?

@carolinan
Copy link
Contributor

If we don't target all inputs, there will be requests for adding more.
Unless a filter is created so that developers can add elements: #64362

'cite' => 'cite',
'caption' => '.wp-element-caption, .wp-block-audio figcaption, .wp-block-embed figcaption, .wp-block-gallery figcaption, .wp-block-image figcaption, .wp-block-table figcaption, .wp-block-video figcaption',
'cite' => 'cite',
'textInput' => 'textarea, input:where([type=email],[type=number],[type=password],[type=search],[type=tel],[type=text],[type=tel],[type=url])',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the text area and inputs bundled together as textInput, there is no way to for example have different padding in a text area compared to a text input, right? And I think we need to be able to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Post Comments Form Affects the Comments Form Block [Type] Enhancement A suggestion for improvement.
Projects
Status: In Dev
Status: Punted to 6.5
Development

Successfully merging this pull request may close these issues.

6 participants