| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | field_ui.module |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | API change, Fields in Core, string freeze, Usability |
Issue Summary
This is a breakout issue from #516138-139: Move CCK HEAD in core.
I still think that the first screen upon adding a field is confusing, but otherwise adding, configuring and deleting fields seem to work all as expected to me. If I was not clear above, I'd repeat this initial screen issue:
- add a field to the article content type
- you got redirected to a page with settings for the field
- BUT the field has no global settings, and even though we are apparently under the Article content type (see page title), Article specific settings are not displayed either
- go to manage fields again and to edit the particular field and now you get both Article specific settings and actual global settings (which in the previous screen were said to be nonexistent)
Initial screen on adding the field:
Going back to the "same" screen in the navigation:
I'm not going to push for solving this before committing, but it is certainly a very core confusion point IMHO.
Comments
#1
#2
I'm attaching a patch to fix the title issue, but ideally the first screen should be skipped completely when there are no applicable settings.
#3
#4
The last submitted patch failed testing.
#5
rerolled patch
#6
#7
Sorry, but requires screenshot
#8
Let's also remove that check_plain() w/ drupal_set_title() since it would be run through that function twice.
#9
check_plain() removed.
#10
@lambic How can you not make a screenshot, with check_plain() removed? It should be beautiful now :P
#11
+ $title = isset($instance['title']) ? $instance['title'] : $instance['label'];I might be missing something, but I don't see where $instance['title'] would come from ?
#12
finally got around to screenshots:
#13
Applied the patch in #9 and this a definite improvement.
#14
#9: 552604.patch queued for re-testing.
#15
The last submitted patch, 552604.patch, failed testing.
#18
Back looking at this workflow again after awhile, I really think this is a bad enough problem we ought to see if we can do something to smooth it out before release. What, I don't really know, since it's quite late. But while it might make sense to Field API folks why adding a field results in two settings screens shown right after another, the second with exactly the same options as the first (only at the bottom this time for extra pizazz!), I can assure you that no one else on the entire planet will understand why this happens.
Over at #867968: Field UI settings workflow is dumb, Bojhan writes:
To me, #4 - the fact that there are two settings pages, with no remotely obvious difference between the two - is the biggest UX wtf here. I do not understand why the first one is even needed, since presumably if I navigated away from the first settings form without saving it, my entire site would not explode, which means that Field API must be doing something smart under the hood in terms of defaults. But the rest of those sound good, too. Separate issues might make sense, but let's come up with a plan first.
Can someone remind me again why we have two settings pages in the first place, and can't just immediately show the widget configuration form assuming the defaults from the field?
#19
Sigh. I'm already buried deep down in #616240: Make Field UI screens extensible from contrib - part II ...
#20
Part of the issue is #821390: Fields usable before all steps to create them are completed. : The forms you go through when creating a field are in fact just the edit forms for the field and the instance. You're editing a field that has been created with default settings and properties the moment you pressed 'Save' on the first screen ('Manage fields')
#21
Yep, agreed there on the edit form thing. So forcing users through this workflow is really awkward, as we don't do this elsewhere in core (congratulations, you just created something! here, now edit its settings. and now edit MORE of its settings!). I would say just ditch the redirection to the settings forms altogether except for the fact that it's not at all clear that clicking a link titled "Autocomplete term widget (tagging)" will take you to a settings form, so I'm not sure how someone would ever find their way back there (actually, that's true even now).
What was the reason again that we can't just stick all of the settings on admin/structure/types/manage/article/fields/field_tags like it was with CCK, instead of having 3 separate settings pages? Then that at least would mesh better with our existing UX elsewhere, which is "to change something's settings, click the edit link next to it".
#22
- separate form to change the widget type:
Because the current widget has an effect on the content of the instance settings - back then #ajax was not where it is today.
This being said, I've been thinking lately that we could let users pick and change the widget with a select dropdown straight from the 'Manage fields' table, rather than linking to a separate form that has a single select.
- separate form for field settings:
Because in D6 everybody screamed that the one-form-including-field-and-instance-settings was too damn big and scary.
Also, in D7, with storage abstraction niceties, some *field* settings (the ones that would affect the schema) cannot be changed after the field creation. While you can change anything at the instance level (doesn't affect schema anyway).
This being said, when doing the initial port of D7 CCK in contrib, Karen left a copy of the 'field' settings form at the bottom of the instance settings form, and this was left as is in #516138: Move CCK HEAD in core...
I've been willing to write a patch to remove that bottom 'field settings' fieldset for weeks :-/.
Also of note:
The 'instance settings' form actually contains
- widget settings, that only affect the how the field is edited, and which can vary depending on the selected widget
- and instance settings : properties of the field that can vary from a content type to the other (required, label, allowed file extensions...), that will be there no matter what the widget)
Those two garbled and intertwined in the same fieldset.
While working on #516138: Move CCK HEAD in core, I advocated for a clearer separation of the two, but quicksketch voiced against.
#23
Cool, thanks for the brain-dump, yched. I'll do some thinkering.
Anyone else, feel free to chime in with suggestions, too.
#24
Thx webchick, much appreciated :-)
#25
Probably not of much help here, but I think:
+1 for clearly separating field from instance from widget settings. If there's a technical design concept behind it, then it doesn't make sense to try to hide that concept in the UI. First source of confusion.
+1 for either directly jumping to instance settings or back to "Manage fields" overview page after creation. The field settings page is the least interesting, and changing configuration options most probably is a rare action anyway, as you're performing site-wide changes to the field.
In general, I think that we badly need to clarify the on-screen tips and descriptions, so as to make users understand when they ought to re-use an existing field, and in which common cases it isn't really a good idea. I'm struggling myself in trying to explain it.
+1 for more #ajax. However, given that the other ajaxified field formatter settings patch was relatively simple, we would have to start with "ideal world mockups" here.
#26
Can we distill what's still possible to do for D7? Some of the sub-issues raised here are indeed major, some others are too late for D7.
#27
subscribe
#28
Remove confusion.
#29
Additionally resolving:
#30
#31
The last submitted patch, drupal.field-instance-settings.29.patch, failed testing.
#32
Much as I would love to say otherwise, I honestly don't think we can do anything about this in D7 now. We're post-RC. :(
#33
I'd agree, if it wouldn't be technically, fundamentally wrong to expose the field settings form on the instance settings form. Most field settings of most fields cannot be (and must not be) changed, so exposing the field settings on that form in the first place makes absolutely no sense at all.
...not to mention that, during debugging of the List field patch just recently, @grendzy reported that you *can* change field settings on the instance settings form without getting a form error... (rather off-topic - actually needs to be a separate issue, because, regardless of where the field settings are getting exposed, we need to ensure that they cannot be changed)
#34
Is the patch you have uploaded reviewable? (it's failing tests but not sure if those are academic failures or horrible problems failures.) I'd need to evaluate the UI impact to make a call on this, I think. If the patch isn't reviewable, I'd be fine with just a general old vs. new workflow description, too.
#35
(if webchick approves this, I'm all for it - thanks sun !)
#36
The patch works and can be applied. The test failures only seem to be caused by test code that expects the field settings on the instance settings form, so fixing those should be relatively easy.
#37
@yched: Manual testing of this patch reveals that the field settings on the instance settings form currently contain the "Number of values" configuration option... which is not contained in the dedicated field settings form... so does that belong into field settings or instance settings? ;)
#38
Cardinality is a field-level setting. it does belong in the 'field settings' form.
#39
Alright, so:
1) Moved the "Maximum number of values" setting that was previously hard-coded into the instance settings form into the field settings form, where it's supposed to be.
2) Due to 1), now it makes sense to always show the field settings form as first step after creating a new field, even when the field itself has no settings. Thus, reverted that change from the previous patch and added an inline comment instead.
The field settings form behavior for fields that have stored data already is a bit weird, as it outputs a message that field settings can no longer be changed, but actually does not prevent the form from submitting. That's a different problem and should be tackled in a separate issue.
#40
The last submitted patch, drupal.field-instance-settings.39.patch, failed testing.
#41
should take care of a few fails.
#42
The last submitted patch, field_ui_instance_settings_form-552604-41.patch, failed testing.
#43
Fixed some more tests.
#44
Screenshots (with patch applied).
#45
Sorry, screenshots in #44 actually map to attached patch.
#46
#45: drupal.field-instance-settings.45.patch queued for re-testing.
#47
Gave it a final test and code review, this is good to go.
Summary :
In current HEAD, Field-level settings are present in both
- The 'Field settings' page, that displays a warning that some settings are not changeable any more when the field has data. The 'Number of values' select dropdown is missing there.
- In a separate fieldset at the bottom of the 'Instance settings' page. The 'Number of values' property is displayed, but not the warning about unchangeable settings. Attempts to change an 'unchangeable setting' (in case the field type module fails to disable the corresponding form element) are not detected there and can produce unpredictable effects.
The patch :
- removes the 'Field settings' fieldset at the bottom of 'Instance settings' page. Field settings are on 'Field settings', instance settings are on 'Instance settings'.
- adds back the missing 'Number of values' select in the 'Field settings' page. This also prevents the WTF effect when creating a field with a type that has no settings, and stumbling on a 'Field settings' step with nothing to set.
#48
So I spent about an hour with this patch tonight with backup from sun on IRC.
Things I like about this patch:
- There are no longer two places to configure fields. That was dumb.
- One of said places is no longer NOT validated. That was really dumb.
Things I don't like about this patch:
- It doesn't solve the actual issue. Adding a new field still leads to a confusing "Field settings" form all by itself. And now it's worse because we have another confusing "Field instance" form all by itself and we force site builders to understand Field API internals enough to know the difference.
- It moves around the cardinality setting which breaks UI freeze, several books, and various documentation, not to mention 1000s of existing D7 sites who expect to find that setting on the second form and not the first, and have since CCK D5. (or maybe even 4.7)
- Because it moves this setting, which can always be changed, to a form that displays a "Settings here cannot be changed" error message after data is added to the field, it introduces a UX bug where instead of this message usually lying to people, now it will always lie to them.
So on the whole, it's about a wash. :\
This is what I would really, really like us to do instead:
1. Actually remove the field settings form. It's not needed. The field gets created in the database with default schema values without me ever having to save that form.
2. Add validation to the field settings form that's living at the bottom of the field instance form, along with all the other settings. It's definitely a nasty bug that that isn't happening.
3. Instead of displaying that error string as a non-targeted dsm('error') that lies half the time, append it to the field description instead in the case where it's a disabled field.
When I suggested doing this in IRC, sun said that the only way we could do this is to also cram in the widget settings on the same form, which would be another big change and likely one better suited for D8. Though this might be nice, I actually disagree that the widget settings form needs to move. Why? Because when you add a field, the "field stuff" and the "widget stuff" are already two separate concepts in the user's mind:
So two separate links going to two separate settings forms kind of makes sense. At least in the "what we can reasonably do for D7" world. (In D8 we should probably re-do this entire thing to be more WYSIWYG or... something else better.)
What they are not exposed to here though is the difference between a "field" and a "field instance". And this level of detail is totally irrelevant for them to be exposed to until much later in their learning process when they discover you can re-use fields among bundles and they can come up with an actual use case for doing that. I think of all the Drupal sites I've ever built, I've used this feature maybe 5 times. So forcing this knowledge on hapless, non-Field API guru site builders who are still trying to wrap their heads around the fact that content types are "entities" and you can add "fields" to "entities" and here's what "fields" are is way, way too much. Hence the "major" priority of this issue, and hence why I don't like sun's fix. It only makes this problem worse.
Because, if you try and write hands-on instructions for a complete newbie on how to add a field to a node type in HEAD, you end up writing something like this:
1. Go to admin/structure/types and click "manage fields" next to "Basic page".
2. Under "Add new field," fill in the following values, and click Save:
- Label: Number field
- Name: number
- Field type: Decimal
- Widget: Text
3. At the first field settings form, choose a precision value of 16 and click Save field settings
4. At the second field settings form, enter the allowed values of "1.030" and "3.203" and click Save settings
It's really, really sad.
If there's only two forms for them to care about here -- one for "everything to do with fields" and one for "everything to do with widgets" -- it radically simplifies things and also matches more closely to what users are used to from CCK.
#49
Ok, after a detailed IRC session, here's the plan sun and yched and I agreed to.
The changes #48 asks for is too much for D7 as the UI change would be pretty drastic and yched has concerns with it. Similarly the patch in #45, though technically correct, also alters the UI too much for D7 -- the "all in one" field settings form gets removed, which many users depend on since it's been there since early versions of CCK.
So we've decided to (mostly) respect UI freeze, and scale back to only fix the actual bugs in this issue, which means:
1. Add the cardinality selector to the solo field settings form. This is a field-level setting; changing it once will change it everywhere, so it makes sense to show it with the rest of the field-level settings.
2. Fix the lack of validation on the "all in one" field settings form. If there are fields that need to be "locked" once the field has data in it, they should be #disabled.
3. Fix the just plain wrong message that says none of the field settings values are changeable once the field has data in it. Cardinality, for example, will always be changeable with field_sql_storage. Replace with a more loosey-goosey worded one like "Because this field has data in it, some of its settings may be locked and unchangeable." This message needs to appear both at the top of the field settings form and on top of the fieldset in field instance form.
#50
Yeah. The original idea was to have a field settings form that had only the unchangeable stuff on it and show only the changeable stuff on the regular edit form, but when that was created there was no way to differentiate between them, so *temporarily* they were shown in both places. And that never got fixed. And the message about not changing settings would have made sense if it was done that way -- you can't change the stuff on the unchangeable form, you can change the stuff on the regular edit form. And since the cardinality is changeable, it went on the regular edit form.
I also thought it would be nice to add information about the widget to the manage fields screen, which we don't have in D6, with a link to change the widget, and remove that part from the global edit form.
So that's more or less the 'why it was done this way' originally.
#51
In other words, the idea was to not say anything about 'field settings' or 'instance settings', just differentiate based on settings that can or cannot later be changed, with all the settings that are changeable and non-problematic on the big edit form and the others tucked away into the field and widget forms.
#52
Hm. Well #50 makes a heck of a lot of sense. So basically just remove the non-changeable fields from the all-in-one form and make the distinction between the two forms "set 'em up once and never change 'em" vs. "configure 'em any time" rather than "field settings" vs. "field instance settings"? Cool!
I think the problem though from talking to yched is that we don't have an easy, standard way of knowing what fields are changeable and what are not. It'd be the responsibility of the field module author to get this right, and they probably won't. Hence the confusingly worded error message. And some are not quite black and white. For example, list items' allowed values. It's fine to add new ones after the field has data, but not ok to change the keys of a list item that has data associated with it. (Or so is my understanding.)
Still though, Karen's approach is even easier if we can figure out a way to do it.
#53
Subscribing. I have read the entire issue, but i'm not clear on if this will / can result in removing the completely pointless first Field settings page when there are no field settings.
Namely, this screen:
#54
It does not remove it, no. :( I tried to summarize in #48.
And this issue is fully D8 now, although a D7 appropriate patch (separate issue) would be fixing the validation of the second copy of the field settings form on the instance settings page.
#55
sub
#56
For anyone who's interested, I implemented @webchick's preferred idea in #48 (getting rid of the separate "field settings" form entirely) as part of the Backports module in Drupal 7.
See: #1277270: Remove the separate field settings page
The code as written probably isn't that useful for an eventual Drupal 8 patch, but I thought I'd mention it anyway. (It also may be useful if anyone wants to click around and see what the UI would look like if we wind up going in that direction here.)
#57
In my own testing of it, it mostly seemed to work well, but I saw some issues when creating List fields.
There, the current workflow of having two forms actually makes a lot of sense. On the first form, you enter the allowed values (which is basically the most important thing you want to focus on when adding a List field), and then when you get to the second form they are already saved, which means that you are able to select one of them in the "default value" dropdown if you want to.
When everything is on one page, though, the allowed values are hidden down at the bottom of the form, and furthermore, since they don't exist yet, you have no options that you can choose from for the "default value". You have to enter the allowed values at the bottom of the form, save the field, then go back and re-edit if you want to set a default value. This is not a great user experience.
#58
There are other kinds of fields where decisions made on the first form affect what appears on the second, esp anywhere that those field settings affect the choices made later (or when those choices affect the way the default value is constructed). Also we have some settings that can later be changed and some that cannot, and it makes sense to have them in separate places, and for the unchangeable settings to be somewhat hidden by not being on the main edit form. And the other issue is that the main edit form is already really too long for good UX, I don't think we want to make it longer by adding back all the things that got moved off of it.
I think the fundamental problems are 1) some fields have no settings and we should really not make a stop on the settings form when a new field is created if there is nothing to do/choose, 2) some values are showing up in two places but they should show up in only one, and 3) The first form should really be a somewhat hidden form that is limited to values that cannot or should not be altered after there is data and the second form, which is easly discovered, should include values that can safely be changed even if there is already data.
#59
Could these types of dependency problems not be mitigated with the use of AJAX or the #states system, though?
#60
Yes, AJAX could fix that probably. But there's the question of order too (what fields should appear next to each other to make the relationships clear).
It's worth pointing out that any problem along these lines is actually a pre-existing bug/issue, just one that would be more noticeable after this change. (Because even in D7, for example, after a field is created you can go back and change its allowed values later, and the default value options will not update to reflect those until the next page load.)
#61
I am not quite sure what to review here?