Dispatch OnBeforeInputInsertText Event in case of number input text-box spin button [chromium/src : main]
Siye Liu (Gerrit)
Siye Liu added 5 comments![]( "Open in Gerrit")
if (DispatchBeforeInputInsertText(Consider adding test cases for both scenarios. We want to test that the beforeinput event is fired after invoking input element event handler.
Here is a reference on how to invoke event handler for input element: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element_test.cc;l=224-238
current.ToString()) != DispatchEventResult::kNotCanceled) {This is the current value. I believe we want to put the new value in `beforeinput` event.
if (DispatchBeforeInputInsertText(event.target()->ToNode(),We should dispatch beforeinput event before changing the value. I believe the value changed in `SpinButtonStepUp` or `SpinButtonStepDown`. Consider moving the logic before calling those two methods.
event.KeyEvent()->text) !=I believe the text inside key event is either `ArrowUp` or `ArrowDown` or `ArrowRight` or `ArrowLeft`. What we want is the value that is going to be set in the input element.
Related details
- Utkarsh Pathak
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Utkarsh Pathak (Gerrit)
Utkarsh Pathak added 4 comments![]( "Open in Gerrit")
current.ToString()) != DispatchEventResult::kNotCanceled) {This is the current value. I believe we want to put the new value in `beforeinput` event.
Moved it to different method.
if (DispatchBeforeInputInsertText(event.target()->ToNode(),We should dispatch beforeinput event before changing the value. I believe the value changed in `SpinButtonStepUp` or `SpinButtonStepDown`. Consider moving the logic before calling those two methods.
Both of these methods make calls to StepUpFromLayoutObject and this method makes call to ApplyStep which is responsible for creating the new value, so I am moving the dispatching of onbeforeevent to ApplyStep so I can have new value calculated directly and we can make this call just before updating the DOM.
event.KeyEvent()->text) !=I believe the text inside key event is either `ArrowUp` or `ArrowDown` or `ArrowRight` or `ArrowLeft`. What we want is the value that is going to be set in the input element.
'''arrowup'''
I believe the text inside key event is either `ArrowUp` or `ArrowDown` or `ArrowRight` or `ArrowLeft`. What we want is the value that is going to be set in the input element.
The key parameter(out of the 4 values you mentioned) can be fetched by the event.key() and event.KeyEvent()->text holds the number/data. currently holded in the text box. Though we should send the new value so I am updating the change and moving the code from here to ApplyStep method where we can directly send the new value.
Related details
- Anupam Snigdha
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 2 comments![]( "Open in Gerrit")
if (DispatchBeforeInputInsertText(Since we move the logic to `InputType::ApplyStep`, please double check if all the callers of this method are expecting the beforeinput event.
if (DispatchBeforeInputInsertText(
EventTargetNodeForDocument(&GetElement().GetDocument()),
new_value.ToString()) != DispatchEventResult::kNotCanceled) {
return;
}The code looks reasonable. Can we move the code down to `// 12....`?
Related details
- Anupam Snigdha
- Rakesh Goulikar
- Sanket Joshi
- Utkarsh Pathak
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anupam Snigdha (Gerrit)
Anupam Snigdha added 1 comment![]( "Open in Gerrit")
Dispatch OnBeforeInputInsertText Event in case of number input text-box spin buttonPlease see https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/contributing.md#Uploading-a-change-for-review on how to write good commit messages. Below is the general format:
```
Summary of change (one line)
Longer description of change addressing as appropriate: why the change
is made, context if it is part of many changes, description of previous
behavior and newly introduced differences, etc.
Long lines should be wrapped to 72 columns for easier log message
viewing in terminals.
Bug: 123456
```
Related details
Anupam Snigdha (Gerrit)
Anupam Snigdha added 2 comments![]( "Open in Gerrit")
DispatchBeforeInputInsertText(What happens if the `ExecutionContext` is deleted during this event by the JS?
Should we check that case and bail out if the `ExecutionContext` is not available?
if (DispatchBeforeInputInsertText(Consider adding test cases for both scenarios. We want to test that the beforeinput event is fired after invoking input element event handler.
Here is a reference on how to invoke event handler for input element: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element_test.cc;l=224-238
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
- Utkarsh Pathak
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Utkarsh Pathak (Gerrit)
Utkarsh Pathak added 6 comments![]( "Open in Gerrit")
Dispatch OnBeforeInputInsertText Event in case of number input text-box spin buttonPlease see https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/contributing.md#Uploading-a-change-for-review on how to write good commit messages. Below is the general format:
```
Summary of change (one line)Longer description of change addressing as appropriate: why the change
is made, context if it is part of many changes, description of previous
behavior and newly introduced differences, etc.Long lines should be wrapped to 72 columns for easier log message
viewing in terminals.Bug: 123456
```
Sure, I have gone through it, it was of great help and I would try to accommodate these practices in future.
Since we move the logic to `InputType::ApplyStep`, please double check if all the callers of this method are expecting the beforeinput event.
It gets called from two places but with different flags so I have put a condition to check this flag to same as we suspect.
if (DispatchBeforeInputInsertText(
EventTargetNodeForDocument(&GetElement().GetDocument()),
new_value.ToString()) != DispatchEventResult::kNotCanceled) {
return;
}The code looks reasonable. Can we move the code down to `// 12....`?
Moved.
DispatchBeforeInputInsertText(What happens if the `ExecutionContext` is deleted during this event by the JS?
Should we check that case and bail out if the `ExecutionContext` is not available?
I don't see situations when the ExecutionContext will be deleted, can you share a few examples where ExecutionContext gets deleted by the JS?
if (DispatchBeforeInputInsertText(Anupam SnigdhaConsider adding test cases for both scenarios. We want to test that the beforeinput event is fired after invoking input element event handler.
Here is a reference on how to invoke event handler for input element: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element_test.cc;l=224-238
+1.
Added Testcase for Keyboard event case and working mouse case.
event.KeyEvent()->text) !=Utkarsh PathakI believe the text inside key event is either `ArrowUp` or `ArrowDown` or `ArrowRight` or `ArrowLeft`. What we want is the value that is going to be set in the input element.
Utkarsh Pathak'''arrowup'''
Passing the correct number in text field to the dipatcher method.
Related details
- Anupam Snigdha
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anupam Snigdha (Gerrit)
Anupam Snigdha added 1 comment![]( "Open in Gerrit")
DispatchBeforeInputInsertText(Utkarsh PathakWhat happens if the `ExecutionContext` is deleted during this event by the JS?
Should we check that case and bail out if the `ExecutionContext` is not available?
I don't see situations when the ExecutionContext will be deleted, can you share a few examples where ExecutionContext gets deleted by the JS?
An example of this scenario is https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/editing/input/edit-context.html;drc=934a749c80854867a92800d5142e25a6108563a7;l=612
You will probably have to make some tweak to this example by adding input text-box spin button inside the iframe etc.
```
const child = document.createElement("iframe");
document.body.appendChild(child);
const childDocument = child.contentDocument;
const editable = childDocument.createElement('div');
editable.contentEditable = true;
childDocument.body.appendChild(editable);
editable.addEventListener("beforeinput", e => {
childEditContext.addEventListener("beforeinput", e => {
child.remove();
});
});
editable.focus();
child.contentWindow.focus();
```
Related details
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
- Utkarsh Pathak
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 3 comments![]( "Open in Gerrit")
// crbug.com/40948436Nit: Move the comment outside of the test case:
```
// crbug.com/40948436
TEST_F(HTMLInputElementTest,
NumberInputOnBeforeInputEventFiringForKeyBoardEvent) {
```
"document.getElementById('ele').addEventListener('beforeinput', "What happened if the event is prevent defaulted? Can you add test case for that?
auto& keyboard_event =Nit: `auto` is used here but real type is used elsewhere. Please consider using either way consistently.
Related details
- Rakesh Goulikar
- Sanket Joshi
- Utkarsh Pathak
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anupam Snigdha (Gerrit)
Anupam Snigdha added 1 comment![]( "Open in Gerrit")
DispatchBeforeInputInsertText(Can we add a WPT test as well to verify that `beforeinput` event is dispatched correctly for this case?
Siye Liu (Gerrit)
Siye Liu added 7 comments![]( "Open in Gerrit")
Nit: Move the comment outside of the test case:
```
// crbug.com/40948436
TEST_F(HTMLInputElementTest,
NumberInputOnBeforeInputEventFiringForKeyBoardEvent) {
```
Done
"<div id='editableText'></div>");Nit: Even thouch the name is "editableText", the div is non-contenteditable. Would be better to rename it to something like "log".
"document.getElementById('ele').addEventListener('beforeinput', "What happened if the event is prevent defaulted? Can you add test case for that?
Done
Nit: `auto` is used here but real type is used elsewhere. Please consider using either way consistently.
Done
number_input.Focus();Can we also check the value in the input element before we fire keyboard event?
It will show that the value won't change if beforeinput event is preventdefaulted.
EXPECT_NE(1, number_input.valueAsNumber());Should this be 0?
if (DispatchBeforeInputInsertText(Anupam SnigdhaConsider adding test cases for both scenarios. We want to test that the beforeinput event is fired after invoking input element event handler.
Here is a reference on how to invoke event handler for input element: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element_test.cc;l=224-238
Utkarsh Pathak+1.
Added Testcase for Keyboard event case and working mouse case.
- Anupam Snigdha
- Rakesh Goulikar
- Sanket Joshi
- Utkarsh Pathak
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Utkarsh Pathak (Gerrit)
Utkarsh Pathak added 4 comments![]( "Open in Gerrit")
Nit: Even thouch the name is "editableText", the div is non-contenteditable. Would be better to rename it to something like "log".
Makes sense, changed the ID to"log"
Can we also check the value in the input element before we fire keyboard event?
It will show that the value won't change if beforeinput event is preventdefaulted.
Added this value check in all 4 TCs
EXPECT_NE(1, number_input.valueAsNumber());Utkarsh PathakShould this be 0?
Here I am checking EXPECT_NE so basically if the event was successfully prevent defaulted then it means that the value shouldn't be increased and changed to 1
Related details
- Anupam Snigdha
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 1 comment![]( "Open in Gerrit")
DispatchBeforeInputInsertText(What happens if the `ExecutionContext` is deleted during this event by the JS?
Should we check that case and bail out if the `ExecutionContext` is not available?
Anupam SnigdhaI don't see situations when the ExecutionContext will be deleted, can you share a few examples where ExecutionContext gets deleted by the JS?
An example of this scenario is https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/editing/input/edit-context.html;drc=934a749c80854867a92800d5142e25a6108563a7;l=612
You will probably have to make some tweak to this example by adding input text-box spin button inside the iframe etc.
```
const child = document.createElement("iframe");
document.body.appendChild(child);
const childDocument = child.contentDocument;
const editable = childDocument.createElement('div');
editable.contentEditable = true;
childDocument.body.appendChild(editable);
editable.addEventListener("beforeinput", e => {
childEditContext.addEventListener("beforeinput", e => {
child.remove();
});
});
editable.focus();
child.contentWindow.focus();
```
- Anupam Snigdha
- Rakesh Goulikar
- Sanket Joshi
- Utkarsh Pathak
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anupam Snigdha (Gerrit)
Anupam Snigdha added 4 comments![]( "Open in Gerrit")
"document.getElementById('ele').addEventListener('beforeinput', "
" () => "
"document.getElementById('log').textContent='beforeinput event "
"was fired!');");Can we add this scenario as a web test/WPT?
DispatchBeforeInputInsertText(Utkarsh PathakWhat happens if the `ExecutionContext` is deleted during this event by the JS?
Should we check that case and bail out if the `ExecutionContext` is not available?
Anupam SnigdhaI don't see situations when the ExecutionContext will be deleted, can you share a few examples where ExecutionContext gets deleted by the JS?
Siye LiuAn example of this scenario is https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/editing/input/edit-context.html;drc=934a749c80854867a92800d5142e25a6108563a7;l=612
You will probably have to make some tweak to this example by adding input text-box spin button inside the iframe etc.
```
const child = document.createElement("iframe");
document.body.appendChild(child);
const childDocument = child.contentDocument;
const editable = childDocument.createElement('div');
editable.contentEditable = true;
childDocument.body.appendChild(editable);
editable.addEventListener("beforeinput", e => {
childEditContext.addEventListener("beforeinput", e => {
child.remove();
});
});
editable.focus();
child.contentWindow.focus();
```
+1
Done
Can we add a WPT test as well to verify that `beforeinput` event is dispatched correctly for this case?
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
- Utkarsh Pathak
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 2 comments![]( "Open in Gerrit")
<!DOCTYPE html>Nit: Consider renaming this file name with more details about the test case.
eventSender.keyDown('ArrowUp');It'd be good to verify that `inputElement` and `child` are gone after this line.
Related details
- Rakesh Goulikar
- Sanket Joshi
- Utkarsh Pathak
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |