Support paste verification

[puntope]

Changes proposed in this Pull Request:

Closes #1001 .

Added functionality in order to allow the user to paste the code inside the verification code control inputs.

Screenshots:

Screen.Recording.2021-11-23.at.10.33.53.mov

Changes

• Mainly a refactor in verification Code Control
• Added a new handlePaste function to handle the input in the case is a paste event
• Added some tests

Detailed test instructions:

1. Go to Onboarding Setup Dashboard or to Settings -> Edit Phone
2. Edit the phone and click on Verify Phone Number
3. Check you can paste digits on the inputs
4. Check you can insert the input as normal
5. You cannot insert letters or symbols, only digits.

Changelog entry

Improve accessibility in Verification Code Control

[eason9487]

Sorry I haven't looked at the .test.js part. This review only comments on the implementation first.

1. 🚧 When pasting, the <input> value of the last digit is incorrectly displayed back to the previous value till losing focus.
   
   
2. ❓ When the length of the pasted codes are not long enough to fill to the last <input>, it should be better to move the focus to the next <input> to be entered
   

[eason9487]

❓
We don't need to handle content other than plain text. Not sure why it needs to use onPaste event? The same newDigits value could be got in the original onInput handler by

const handleInput = ( e ) => {
	const { value, dataset } = e.target;
	const newDigits = value
		.replace( /\D/g, '' )
		.substr( cursorRef.current, DIGIT_LENGTH - idx );

	// ...omitted
};

I think this could simplify the processing of getting content from onPaste, and even further simplify other changes by using the original handleInput to handle a 1 to DIGIT_LENGTH length array of digits.

[eason9487]

FYI - If need to get more context from the previous relevant discussions, you can find them in these PRs:

• Implement new components for phone verification process #993
• Fix issues and react warnings for verification code components #1007

[puntope]

2. ❓ When the length of the pasted codes are not long enough to fill to the last <input>, it should be better to move the focus to the next <input> to be entered

Agree! 👍

Fixed in e37f3b7

[puntope]

1. 🚧 When pasting, the <input> value of the last digit is incorrectly displayed back to the previous value till losing focus.

This is definitely working for me. 😕 Could you double-check?

Note: At some point in the dev process, it wasn't working, that is why I added the statement updateInputRefs( digits );

[eason9487]

1. 🚧 When pasting, the <input> value of the last digit is incorrectly displayed back to the previous value till losing focus.

This is definitely working for me. 😕 Could you double-check?

Tried with npm install, restarting npm start, and the latest version of Chrome, the problem can be reproduced on my end. A new finding is that it only occurs during the cooling countdown, which means that it probably occurs when any re-renders are triggered from upper layers.

[puntope]

A new finding is that it only occurs during the cooling countdown, which means that it probably occurs when any re-renders are triggered from upper layers.

First of all, thanks for catching this @eason9487! 🙏 My manual test was wrong cos my counter was off due request limit. Now I was able to reproduce and fix the error.

the problem

Bug causes:

Testing deeper I can confirm the problem is related to the way input-field handles the internal state on focus after each render. The value is not updated in the render until the input lost focus. I could confirm that everything works fine replacing that ìnput-field` with a native input.

On single-digit insertion, we don't have that problem since we move the focus to the next input box. Hence, the previous one is updated correctly after losing focus.

Solution:

I replaced now the inputRefs update solution with a new state solution, in order to create an extra tick and sync the internal state for the input-field

Besides that, I refactored a bit the resetNeedle effects in order to avoid sync conflicts.

Let me know if it works now :)

Tested:

• Works with counter running both insert and paste, full and partial pastes.
• Change on resetNeedle reset the digits and the focus. (Click on resend, change method, request error)
• Works with counter stoped both insert and paste, full and partial pastes.
• Verification code value in parent component is updated.
• No es-lint warns
• Unit tests passed

[eason9487]

Thanks for the updates!

However, I'm sorry that it brings new issues and some previous issues are still unresolved.

1. 🚧 The original automatic focus for input cursor is still not working when no entered verification codes.
   Switch verification method
   
   Resend
   
2. 🚧 Sometimes the focus stays at the pasting <input> with previous value when pasting filled <input>s. Steps I tried
   1. Paste 123456 on the first <input>
   2. Copy 886 and pate on the third <input>
   3. The displaying values are changed to 123866, but I expected it would be 128866 and the focus should be moved to the last <input>.
      
3. 🚧 When pasting on the last , the problem of the display value is out of sync with the internal state still exists till losing focus.
   

[eason9487]

There're two new critical issues after adding new commits.

[puntope]

However, I'm sorry that it brings new issues and some previous issues are still unresolved.

No sorry, thanks to you for checking this! Better you than the users :) Thank you for being so thorough in the reviews!

🚧 The original automatic focus for input cursor is still not working when no entered verification codes.
Switch verification method

Fixed here d20faae

🚧 Sometimes the focus stays at the pasting with previous value when pasting filled s. Steps I tried
Paste 123456 on the first
Copy 886 and pate on the third
The displaying values are changed to 123866, but I expected it would be 128866 and the focus should be moved to the last .

Fixed here d20faae

🚧 When pasting on the last , the problem of the display value is out of sync with the internal state still exists till losing focus.

Fixed here d20faae

[puntope]

The useRef removed by 875f05e is not unnecessary. It was used for this reason.

Restored!

[puntope]

💅

Use === to check whether the values in two different arrays are equal is not appropriate.

Added toString() strategy. Other alternatives are...

_.isEqual (lodash)
Just a loop
Stringify

[eason9487]

📜
Due to the internal state management of <InputControl>, the implementation of this component has to use some workaround for the focus controlling of input cursor. But the fixing result after several rounds of code review makes the implementation start to become too complex. Even more, workarounds have to be used at more places because of other side effects caused by the added workarounds.

The key problem is when a <InputControl> has the focus, updating the value prop would not affect the actually displaying content on the <input>, and even update via the value property of the real <input> DOM node could temporarily show the wanted content, it will be still recovered back to the content of passing value prop after any re-render occurs.

When we want to set the focus on an unfocused <InputControl>, which is also updating its display content, this state conflict happened. To deal with this, it could be resolved by updating the value prop and yielding the focus move to the next tick of re-render.

And this case will only happen in the first (when reset) and the last (when pasted) <InputControl>. Moving the workaround into the maybeMoveFocus function with a bit of magic could make the implementation simpler and clearer.

const maybeMoveFocus = ( targetIdx ) => {
	const validIdx = Math.max( Math.min( targetIdx, DIGIT_LENGTH - 1 ), 0 );
	const node = inputsRef.current[ validIdx ];

	if ( targetIdx === validIdx ) {
		node.focus();
	} else {
		setTimeout( () => node.focus() );
	}
};

Then we could call maybeMoveFocus( -1 ); when resetting, and maybeMoveFocus( DIGIT_LENGTH ); when moving focus to the last. With this method, it will need to change most other implementations. I made the alternative with code comments here 8db0834...707e15b. Probably we could consider this solution.

[eason9487]

🚧

The focus state may be out of sync with the actual focusing index of <input> nodes, and further causes the input to get stuck due to all deps have not been changed here:

google-listings-and-ads/js/src/components/contact-information/phone-number-card/verification-code-control.js

Lines 198 to 200 in d20faae

	useEffect( () => {
	maybeMoveFocus( focus );
	}, [ digits, resetNeedle, focus ] );

Steps:

1. Enter a number in the first <input>
2. Move back to the first <input> by the ← key or mouse
3. Enter the same number as step 1
4. I expect it would move to the next input but it gets stuck

[eason9487]

❓
The meaning of this code comment is different from the if ( digit !== value ) condition and its processing below.

[eason9487]

💅
The code comments are outdated after changing the position and implementation of the original useEffect.

[eason9487]

📜
I think we don't really need this. I will propose an alternative that simplifies overall implementations by the new submitting code review.