WIP Pattern input field #17472

brunopagno · 2024-12-16T15:37:49Z

Ticket

https://community.openproject.org/projects/document-workflows-stream/work_packages/59999

What are you trying to accomplish?

Screenshots

Screencast.From.2025-01-14.10-36-22.mp4

What approach did you choose and why?

The component was built with rails leveraging stimulus + primer where available. There is minimal custom styling. That said, the component does not look like Figma because we wanted to favour primer components.

A few decisions were made here:

Using a contenteditable field for the input
A few hacks were necessary (e.g. adding spaces around the editable field so that they are still editable)

NobodysNightmare

I've done some testing mostly from the UI side (i.e. not looking at the code first, but checking behaviour first) and left some comments.

Feel free to give me feedback on whether you agree or disagree to the findings, but also whenever you think that addressing a finding takes too much effort and should rather be extracted to a separate PR.

frontend/src/stimulus/controllers/pattern-autocompleter.controller.ts

NobodysNightmare · 2025-01-14T10:15:13Z

frontend/src/stimulus/controllers/pattern-autocompleter.controller.ts

+    }
+
+    // move up and down the suggestions selection
+    if (event.key === 'ArrowUp') {


What I see is that we implement the behaviour of a select box manually here. This is tricky. For example I noticed that when using a screen reader, the current implementation does not read out my selection when I change it using the arrow keys. A normal select box (e.g. the color selection in the settings tab) does this.

That's an accessibility concern as far as I can tell. However, I realize that the same problem can be observed for two other places in our UI: The project selection in the top left and our emoji autocompleter. So maybe we never solved this so far. (for comparison, the GitHub emoji autocompleter behaves fine)

Sadly I am by no means an expert in implementing accessible custom components. As far as I can tell, the usually best practice is to start with a standard component that is already accessible, e.g. using a real select box in the background.

I am not sure how to handle this, though. I will reach out to other people on the dev team to see if it helps.

But I was more afraid of attempting to change focus/force navigate with something that is supposed to be a shortcut.

Maybe there's an obvious solution.

The idea with using Primer was to fully spport accessibility again, as the Primer components are claimed to be accessible. However, the ActionList in Primer is not meant to be used standalone afaik, so we are more or less misusing it here. Looking at other components where the actionList is used, it is possible to navigate via arrows. Primer has an internal helper called focus-group for that, see:

https://github.com/primer/view_components/blob/main/app/components/primer/focus_group.ts

https://github.com/primer/view_components/blob/main/app/components/primer/alpha/action_menu.html.erb#L2

Maybe, we simply have to add that around our ActionList as well and we can get rid of the custom selection logic. But that is just an educated guess, I don't know what that really works.

This commit introduces a new type of input field which can handle autocomplete with 'tokens', so that we can build patterns like `Example pattern {{token}} value`.

NobodysNightmare · 2025-01-14T12:52:06Z

frontend/src/stimulus/controllers/pattern-autocompleter.controller.ts

+    }
+
+    // close the suggestions
+    if (event.key === 'Escape' || event.key === 'ArrowLeft' || event.key === 'ArrowRight') {


(Very optional)

Small suggestion after realizing that using Esc we can close the auto-completer: What do you think about adding Ctrl+Space as a combo to open the list again?

After the latest change this can also be useful for cases where the menu accidentially closed during keyboard navigation.

frontend/src/stimulus/controllers/pattern-autocompleter.controller.ts

NobodysNightmare · 2025-01-14T14:25:25Z

Because the PR template always reminds me about it: Should we add our new component also to the Lookbook?

I am not quite sure how this works, but I guess we need to add a file like lookbook/previews/open_project/common/pattern_autocomplete_preview.rb, maybe not in "common", because our component has a pretty narrow use case. But unsure.

NobodysNightmare · 2025-01-15T07:00:54Z

app/components/work_packages/types/pattern_autocompleter.rb

+
+      def select_item_action
+        {
+          action: "click->pattern-autocompleter#suggestions_select",


I just spotted this one and noticed that I didn't try using the mouse to select an autocomplete entry yet.

For me it doesn't work to click on an autoselect entry. The pop-up closes, but nothing gets selected.

New learning: Selecting with the mouse works after opening the suggestion list via the arrow down button, but not when using the mouse after starting autocompletion via typing.

I shall investigate. Maybe the focus group solution suggested above fixes all things and I can move on with life 👀

HDinger

Hi @brunopagno

I don't know which detail of review you want at that point. I will just write down the issues I found, feel free to ignore when you already aware of that.

When loading the page, the field is shortly shown. Sometimes, it there so long that I even could start typing before it vanished

The search results are only selectable via keyboard, not via mouse click
The label text and closing "x" are not alligned

The labels can overflow the input

When there are no search results, the headlines are still shown, which feels weird

I can't reliably reproduce it, but sometimes there are issues with the cursor position which is jumping around. For example when you select a label as the first element, it jumps to the right after pressing space.

In general it works nicely. Looking at the code, it still feels however as if we should have solved that differently from the UX side from the beginning. We are implementing a lot of custom stuff here and misusing components for that. I know that it is too late now, but we should keep that in mind for later similar cases.

HDinger · 2025-01-15T07:12:05Z

app/components/work_packages/types/pattern_autocompleter.html.erb

+        )
+      ) do
+    %>
+      <%= content_tag(:span, style: "cursor: default;", "data-role": "token-text") { "__VALUE__" } %>


Please do not use inline styles. Either use system_arguments or if the style is not supported, add a class and write the styles in the CSS file.

HDinger · 2025-01-15T07:13:51Z

app/components/work_packages/types/pattern_autocompleter.html.erb

+      <%=
+        render(
+          Primer::Beta::Octicon.new(
+            icon: :x, size: :xsmall, style: "cursor: pointer;margin: 0.2em;",


Same, please do not use inline styles. Further, 0.2 is somehow a magic number, that does not even align the cross to the center:

Please try to resolve that with CSS alignment mechanism (e.g. vertica-align or a flex)

HDinger · 2025-01-15T07:15:30Z

app/components/work_packages/types/pattern_autocompleter.html.erb

+    <% end %>
+  </template>
+
+  <%= content_tag(:div, style: "position: relative;") do %>


Suggested change

<%= content_tag(:div, style: "position: relative;") do %>

<%= render(Primer::BaseComponent.new(tag: :div, position: :relative)) do %>

HDinger · 2025-01-15T07:19:11Z

app/components/work_packages/types/pattern_autocompleter.html.erb

+          Primer::Beta::Octicon.new(
+            icon: "triangle-down",
+            size: :small,
+            style: "cursor: pointer;position: absolute;right: 1em;top: 0.5em;",


Position absolute has some drawbacks in this case. You are enforcing it to lay at that specific place no matter what the content of the input is. Thus the content and the arrow can overlap:

I didn't see that arrow down at all until now 🙈

Devils advocate: Do we need this arrow at all? It makes the component look like a select, when it is mostly like a text input. E.g. we'd also not show an arrow down in a text input that allows autocompleting emojis (which I think is a good analogy for what we are doing here).

The arrow allows for a mouse only interaction of adding tokens so I think it might be necessary. 🤔

HDinger · 2025-01-15T07:24:01Z

app/components/work_packages/types/pattern_autocompleter.sass

@@ -0,0 +1,5 @@
+.pattern-autocompleter
+  .selected
+    color: var(--list-item-hover--color)


Looking at the ActionList component, they use --fgColor-default for hovered elements. As we try to mimic that style, I'd suggest that we use the same here.

HDinger · 2025-01-15T07:46:29Z

frontend/src/stimulus/controllers/pattern-autocompleter.controller.ts

+    }
+
+    // move up and down the suggestions selection
+    if (event.key === 'ArrowUp') {


The idea with using Primer was to fully spport accessibility again, as the Primer components are claimed to be accessible. However, the ActionList in Primer is not meant to be used standalone afaik, so we are more or less misusing it here. Looking at other components where the actionList is used, it is possible to navigate via arrows. Primer has an internal helper called focus-group for that, see:

https://github.com/primer/view_components/blob/main/app/components/primer/focus_group.ts

https://github.com/primer/view_components/blob/main/app/components/primer/alpha/action_menu.html.erb#L2

Maybe, we simply have to add that around our ActionList as well and we can get rid of the custom selection logic. But that is just an educated guess, I don't know what that really works.

HDinger · 2025-01-15T08:24:48Z

Some other things:

When I selected and item from the list via Enter, and the press Enter again, the element is added again to the input
In Safari, after I added an item, the cursor jumps to the beginning of the input
Currently it is not possible to delete a label via keyboard, only with a mouse. That is also an accessibility issue.

brunopagno self-assigned this Dec 16, 2024

brunopagno force-pushed the impl/59999-pattern-autocompleter-input branch 9 times, most recently from f82ef17 to 896dbd3 Compare December 24, 2024 14:04

brunopagno force-pushed the impl/59999-pattern-autocompleter-input branch 5 times, most recently from 8f324e2 to 428ede5 Compare January 7, 2025 08:17

brunopagno force-pushed the impl/59999-pattern-autocompleter-input branch 4 times, most recently from 02b24d2 to 17ccd5e Compare January 10, 2025 11:13

lindenthal added the pullpreview label Jan 11, 2025

github-actions bot temporarily deployed to gh-6899875-pr-17472 January 11, 2025 07:02 Inactive

brunopagno force-pushed the impl/59999-pattern-autocompleter-input branch from 2f587af to e20ace7 Compare January 13, 2025 09:12

github-actions bot temporarily deployed to gh-6899875-pr-17472 January 13, 2025 09:12 Inactive

brunopagno force-pushed the impl/59999-pattern-autocompleter-input branch from e20ace7 to 90e816f Compare January 13, 2025 09:13

github-actions bot temporarily deployed to gh-6899875-pr-17472 January 13, 2025 09:25 Inactive

brunopagno force-pushed the impl/59999-pattern-autocompleter-input branch from 90e816f to e6cd61c Compare January 13, 2025 18:14

github-actions bot temporarily deployed to gh-6899875-pr-17472 January 13, 2025 18:15 Inactive

brunopagno force-pushed the impl/59999-pattern-autocompleter-input branch from e6cd61c to 9f03cd4 Compare January 13, 2025 18:19

github-actions bot temporarily deployed to gh-6899875-pr-17472 January 13, 2025 18:27 Inactive

brunopagno force-pushed the impl/59999-pattern-autocompleter-input branch from 9f03cd4 to 8ca26a9 Compare January 14, 2025 09:35

github-actions bot temporarily deployed to gh-6899875-pr-17472 January 14, 2025 09:36 Inactive

brunopagno marked this pull request as ready for review January 14, 2025 09:41

brunopagno requested review from HDinger, Kharonus and a team January 14, 2025 09:41

brunopagno changed the title ~~WIP pattern autocompleter input~~ Pattern autocompleter input Jan 14, 2025

NobodysNightmare requested changes Jan 14, 2025

View reviewed changes

create a pattern autocompleter input

685b5e1

This commit introduces a new type of input field which can handle autocomplete with 'tokens', so that we can build patterns like `Example pattern {{token}} value`.

brunopagno force-pushed the impl/59999-pattern-autocompleter-input branch from 8ca26a9 to 685b5e1 Compare January 14, 2025 12:43

github-actions bot temporarily deployed to gh-6899875-pr-17472 January 14, 2025 12:44 Inactive

NobodysNightmare reviewed Jan 14, 2025

View reviewed changes

frontend/src/stimulus/controllers/pattern-autocompleter.controller.ts Show resolved Hide resolved

NobodysNightmare reviewed Jan 14, 2025

View reviewed changes

frontend/src/stimulus/controllers/pattern-autocompleter.controller.ts Outdated Show resolved Hide resolved

NobodysNightmare reviewed Jan 14, 2025

View reviewed changes

frontend/src/stimulus/controllers/pattern-autocompleter.controller.ts Outdated Show resolved Hide resolved

adjust javascript implementation details

6c2bfef

github-actions bot deployed to gh-6899875-pr-17472 January 14, 2025 15:02 View deployment

NobodysNightmare reviewed Jan 15, 2025

View reviewed changes

HDinger requested changes Jan 15, 2025

View reviewed changes

brunopagno mentioned this pull request Jan 16, 2025

WIP Pattern input field (the alternative version) #17634

Draft

3 tasks

brunopagno changed the title ~~Pattern autocompleter input~~ WIP pattern autocompleter input Jan 16, 2025

brunopagno changed the title ~~WIP pattern autocompleter input~~ WIP Pattern input field Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Pattern input field #17472

WIP Pattern input field #17472

brunopagno commented Dec 16, 2024 •

edited

Loading

NobodysNightmare left a comment

NobodysNightmare Jan 14, 2025

brunopagno Jan 14, 2025

HDinger Jan 15, 2025

NobodysNightmare Jan 14, 2025

NobodysNightmare commented Jan 14, 2025 •

edited

Loading

NobodysNightmare Jan 15, 2025 •

edited

Loading

NobodysNightmare Jan 15, 2025

brunopagno Jan 15, 2025

HDinger left a comment •

edited by brunopagno

Loading

HDinger Jan 15, 2025

HDinger Jan 15, 2025

HDinger Jan 15, 2025

HDinger Jan 15, 2025

NobodysNightmare Jan 15, 2025

brunopagno Jan 15, 2025

HDinger Jan 15, 2025

HDinger Jan 15, 2025

HDinger commented Jan 15, 2025

	<%= content_tag(:div, style: "position: relative;") do %>
	<%= render(Primer::BaseComponent.new(tag: :div, position: :relative)) do %>

WIP Pattern input field #17472

Are you sure you want to change the base?

WIP Pattern input field #17472

Conversation

brunopagno commented Dec 16, 2024 • edited Loading

Ticket

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

NobodysNightmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NobodysNightmare commented Jan 14, 2025 • edited Loading

NobodysNightmare Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HDinger left a comment • edited by brunopagno Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HDinger commented Jan 15, 2025

brunopagno commented Dec 16, 2024 •

edited

Loading

NobodysNightmare commented Jan 14, 2025 •

edited

Loading

NobodysNightmare Jan 15, 2025 •

edited

Loading

HDinger left a comment •

edited by brunopagno

Loading