Support readonly for all fields#1677
Conversation
| assigns = | ||
| if Map.has_key?(assigns.rest, :disabled) do | ||
| assigns | ||
| else | ||
| put_in(assigns, [:rest, :disabled], Map.get(assigns.rest, :readonly) || false) | ||
| end |
There was a problem hiding this comment.
| assigns = | |
| if Map.has_key?(assigns.rest, :disabled) do | |
| assigns | |
| else | |
| put_in(assigns, [:rest, :disabled], Map.get(assigns.rest, :readonly) || false) | |
| end |
Why do we want to set disabled based on the readonly attribute? If you want to set the input to disabled, you should explicitly pass the disabled attribute, in my opinion.
There was a problem hiding this comment.
See https://github.com/naymspace/backpex/blob/0.16.3/lib/backpex/fields/text.ex#L58
I noticed that we do not set dsiabled in every field (e.g. Backpex.Fields.Select)
There was a problem hiding this comment.
yeah you're right, I'll review your comments to this PR asap (some time has passed and I can't remember right now). Anyways I was confused by the fact that in Backpex almost all fields have
<BackpexForm.input
...
readonly={@readonly}
disabled={@readonly}
>Do you think it makes sense to have two separate options, :readonly and :disabled? After all, they serve different purposes. If that's the case I can work on that and add the :disabled option to Backpex.Field.
| assigns = | ||
| if Map.has_key?(assigns.rest, :disabled) do | ||
| assigns | ||
| else | ||
| put_in(assigns, [:rest, :disabled], Map.get(assigns.rest, :readonly) || false) | ||
| end |
There was a problem hiding this comment.
| trigger_class = (assigns.trigger && assigns.trigger[:class]) || "" | ||
|
|
||
| trigger_class = | ||
| if assigns.readonly do | ||
| ["cursor-not-allowed bg-base-200"] ++ | ||
| (trigger_class | ||
| |> Enum.join(" ") | ||
| |> String.split() | ||
| |> List.delete("bg-transparent") | ||
| |> List.delete("input")) | ||
| else | ||
| trigger_class | ||
| end |
There was a problem hiding this comment.
This looks complicated. Can't we just add additional classes in the markup in case the dropdown is readonly?
There was a problem hiding this comment.
Here is why. This is how the dropdown is rendered with the code above:
Whereas this code:
trigger_class =
if assigns.readonly do
["cursor-not-allowed bg-base-200"] ++ trigger_class
else
trigger_class
endresults in this, and the field can be even highlighted on mouse click (although the dropdown does not open):
| </:label> | ||
| <Form.multi_select | ||
| field={@form[@name]} | ||
| readonly={@readonly} |
There was a problem hiding this comment.
undefined attribute "readonly" for component Backpex.HTML.Form.multi_select/1
| assigns = | ||
| if assigns[:readonly] == true do | ||
| assigns | ||
| else | ||
| assign(assigns, :readonly, Backpex.Field.readonly?(field_options, assigns)) | ||
| end |
There was a problem hiding this comment.
For which cases do we need this?
And can we simplify the if-condition?
| assigns = | |
| if assigns[:readonly] == true do | |
| assigns | |
| else | |
| assign(assigns, :readonly, Backpex.Field.readonly?(field_options, assigns)) | |
| end | |
| assigns = | |
| if assigns[:readonly] do | |
| assigns | |
| else | |
| assign(assigns, :readonly, Backpex.Field.readonly?(field_options, assigns)) | |
| end |
There was a problem hiding this comment.
Sure, the if condition can be simplified.
The reason for this code is because the :readonly assign is not always defined when calling the callback Backpex.Field.render_form/1 and also to allow individual fields in Backpex.Fields.InlineCRUD to be set readonly when the parent InlineCRUD field is not readonly. I will add a comment in the code to clarify this.
The `:readonly` field option, being shared among all fields, has been moved to the `@config_schema` of `Backpex.Field`.
gmazzamuto
force-pushed
the
feature/readonly-fields
branch
from
30ef1e3 to
c2ab647
Compare
Previously the readonly dropdown kept role="button", tabindex=0, and aria-haspopup="true" while stripping the menu, producing a focusable element that announces a popup that isn't there. Also removed the fragile class-token-surgery that reached into caller class lists to delete "input" and "bg-transparent" strings; callers now build their own readonly-aware class lists using Phoenix's list syntax.
The drop target's phx-drop-target attribute was not gated on readonly, so drag-and-drop still started uploads. Cancel buttons for in-progress and existing entries remained clickable, letting users mutate file state. The "Upload a file" <a> rendered as a dead link in readonly mode — now a plain <span> with the interactive classes removed.
text-base-content/40 on bg-base-200 falls below 4.5:1 on stock daisyUI themes. Bumped to /60 to match the codebase's standard muted-text token (used in help_text / labels).
HTML's readonly and disabled have different a11y semantics: disabled
removes the element from the tab order and is frequently skipped by
screen readers in form-review mode, making the readonly value
invisible. For inputs that natively support readonly (text, number,
date, date_time, time, textarea, email, url, currency), keeping only
readonly={@readonly} is correct. Controls without native readonly
(select, multi_select, boolean, relations, upload) continue to use
disabled as before.
Previously the readonly branch removed the delete checkbox but left an orphaned <label for> pointing at nothing, wrapping a <div class="btn btn-disabled"> with sr-only "Delete" text. Screen readers announced a non-interactive "Delete" with a dangling label association. Same problem for "Add entry" — rendered as a disabled checkbox-masquerading-as-button. In readonly mode a user has no use for either control, so they are now omitted entirely.
select_relational_field/1 and pivot_field/1 previously did not receive the parent field's readonly flag. The main UI already hid the triggers that invoke them, so no live regression — but any future path that opens the relation modal (e.g. programmatic push_event, pre-populated newest_relational) would render an editable select on a readonly field. Now both components declare readonly as an explicit attr and forward it to their inputs.
The guide listed only Date, DateTime, Number, Text, Textarea as readonly-capable. With this PR, readonly is a global Backpex.Field option and most field types support it. Documented the three rendering strategies (native-readonly, disabled, custom) and the per-field quirks for Upload, InlineCRUD, HasManyThrough, Boolean.
…ledocs These three fields render readonly with custom UI changes that hide or disable elements beyond the standard readonly/disabled attribute. Added short Readonly sections to the moduledocs so users discover this behavior without reading the guide.
Covers the readonly-branch HTML surface of both components: dropdown has no role/tabindex/aria-haspopup/menu when readonly; multi_select prompt uses /60 contrast and badges render without primary color or remove controls. Sets up test/html/ as the home for future HTML component tests (the repo previously had no component-level coverage).
2 participants
Support
:readonlyoption for all fields.The
:readonlyfield option, being shared among all fields, has been moved to the@config_schemaofBackpex.Field. This should not be a breaking change.Example of readonly fields: