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

User Input: Can't Delete Search Terms with Keyboard #2900

Closed
ivankruchkoff opened this issue Mar 1, 2021 · 7 comments
Closed

User Input: Can't Delete Search Terms with Keyboard #2900

ivankruchkoff opened this issue Mar 1, 2021 · 7 comments
Labels
Good First Issue Good first issue for new engineers P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Milestone

Comments

@ivankruchkoff
Copy link
Contributor

ivankruchkoff commented Mar 1, 2021

Bug Description

When I fill out search terms, I can't then delete them with the backspace/delete key.

Steps to reproduce

  1. Go to user input flow
  2. Get to 5/5 search terms
  3. Fill out some search terms
  4. Try and delete with backspace
  5. See that search terms don't get deleted

Screenshots

Additional Context

  • PHP Version:
  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Plugin Version [e.g. 22]
  • Device: [e.g. iPhone6]

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • When pressing the backspace key within a search term (i.e. cursor is within the search term bubble), it should delete characters from that search term.
  • When pressing the backspace key when right after a search term (i.e. the cursor is outside a search term bubble, right after one), it should delete that entire search term.

Implementation Brief

  • Once Fix bad UX approach for typing search terms in user input flow #2842 has been merged, using /assets/js/components/user-input/UserInputKeywords.js,
    • import BACKSPACE from @wordpress/keycodes.
    • create new function deleteKeyword which does the same thing as onKeywordDelete.
    • update onKeywordDelete to use the deleteKeyword function.
    • update the onKeyDown function to:
      • get the length of nonEmptyValues
      • if the length is less than 3 and the BACKSPACE key is pressed, call the deleteKeyword function passing nonEmptyValuesLength - 1 as the index to delete.
      • set the focus to the input text again using the same logic when the ENTER or COMMA keys are pressed, but this time, having the following selector: #${ slug }-keyword-${ nonEmptyValuesLength - 1 }

Test Coverage

N/A

Visual Regression Changes

N/A

QA Brief

  • Fill out the user input flow until you reach step 5 (search terms)
  • You should be able to:
    • Enter up to 3 words, pressing comma or enter/ return to enter a word
    • Delete within a word you are typing (i.e. hitting backspace while typing a word should delete the character you have just typed)
    • Delete a term after entering it (e.g. type test, hit comma to enter it, then testing, hitting comma again. Now hit the the backspace and testing should be removed, but not test).

Changelog entry

  • Fix accessibility problem where it wasn't possible to delete user input search terms using the keyboard.
@ivankruchkoff ivankruchkoff changed the title User Input: Can't Delete Search Terms User Input: Can't Delete Search Terms with Keyboard Mar 1, 2021
@felixarntz felixarntz self-assigned this Mar 2, 2021
@felixarntz felixarntz added Next Up P1 Medium priority Type: Enhancement Improvement of an existing feature labels Mar 2, 2021
@felixarntz felixarntz removed their assignment Mar 2, 2021
@asvinb asvinb assigned asvinb and unassigned asvinb Mar 9, 2021
@asvinb asvinb added the Good First Issue Good first issue for new engineers label Mar 9, 2021
@fhollis fhollis added this to the Sprint 45 milestone Mar 9, 2021
@tofumatt tofumatt self-assigned this Mar 11, 2021
@tofumatt
Copy link
Collaborator

IB largely looks good but one thing I'm not clear on:

if nonEmptyValues.length is than 3 and the BACKSPACE key is pressed, call the deleteKeyword function passing nonEmptyValuesLength - 1 as the index to delete.

Why does it matter if there are fewer than the max number of keywords entered? Isn't the idea that if you can focus the text field and backspace in an empty bubble/outside a bubble that it deletes no matter what?

I'd think this check would instead be if nonEmptyValues.length is 0—if that's the case we don't delete anything as there's nothing to delete.

@tofumatt tofumatt assigned asvinb and unassigned tofumatt Mar 11, 2021
@eclarke1 eclarke1 removed the Next Up label Mar 15, 2021
@asvinb
Copy link
Collaborator

asvinb commented Mar 15, 2021

@tofumatt As per the AC,
When pressing the backspace key within a search term (i.e. cursor is within the search term bubble), it should delete characters from that search term.
so if you are at the third item (max. number), if the backspace key is pressed, it should not delete the item, rather it should delete characters from that item.

@johnPhillips
Copy link
Contributor

Just realised this IB isn't finished, sorry! I'll pause work until it is passed review. My mistake 🙏

@tofumatt tofumatt assigned johnPhillips and unassigned tofumatt Mar 15, 2021
@tofumatt
Copy link
Collaborator

@asvinb Ah, okay, thanks! Makes sense now. 👍🏻

IB ✅

@tofumatt tofumatt assigned tofumatt and johnPhillips and unassigned tofumatt Mar 25, 2021
@eclarke1 eclarke1 added the Rollover Issues which role over to the next sprint label Mar 29, 2021
@eclarke1 eclarke1 modified the milestones: Sprint 45, Sprint 46 Mar 29, 2021
@johnPhillips johnPhillips assigned tofumatt and unassigned johnPhillips Apr 6, 2021
tofumatt added a commit that referenced this issue Apr 7, 2021
…erms-keyboard

User Input: Can't Delete Search Terms with Keyboard (#2900)
@tofumatt tofumatt removed their assignment Apr 7, 2021
@wpdarren wpdarren self-assigned this Apr 7, 2021
@wpdarren
Copy link
Collaborator

wpdarren commented Apr 7, 2021

QA Update: Fail ❌

@johnPhillips when I hit backspace within a word, it will allow me to delete one letter but the cursor then jumps to the end of the word. This is an issue if I am wanting to delete more than one letter within the word. When I hit the backspace once, I should then be able to hit the key again to delete the next letter.

Right now, I have to click backspace once to delete the letter, and then click in the word again next to the next letter I want to delete, and hit backspace again. Here's a quick screencast

user-input.mp4

Verified:

Delete a term after entering it, is removed. (e.g. type test, hit comma to enter it, then testing, hitting comma again. Now hit the the backspace and testing is removed, but not test).

@wpdarren wpdarren assigned johnPhillips and unassigned wpdarren Apr 7, 2021
@tofumatt tofumatt assigned tofumatt and unassigned johnPhillips Apr 7, 2021
@tofumatt
Copy link
Collaborator

tofumatt commented Apr 7, 2021

@johnPhillips and I chatted about this and turns out this was an issue even before this fix landed—the way we store the data for these elements in state and then select them causes a known bug with input cursor jumping with controlled components. I found a fix for this and will have a PR soon… it's arguably almost a separate issue but I'll fix it as part of the QA here. 🙂

@wpdarren
Copy link
Collaborator

wpdarren commented Apr 12, 2021

QA Update: Pass ✅

On the 5th question, when you enter up to 3 words:

  • User can delete letters within a word.
  • User can delete a term after entering it.
  • Also confirmed that these two tests in the QAB on mobile and tablet devices.
user-input.mov

@wpdarren wpdarren removed their assignment Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants