Dispatch OnBeforeInputInsertText Event in case of number input text-box spin button [chromium/src : main]

0 views
Skip to first unread message

Siye Liu (Gerrit)

unread,
Apr 17, 2024, 11:38:50 AMApr 17
to Utkarsh Pathak, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Utkarsh Pathak

Siye Liu added 5 comments

Patchset-level comments
File-level comment, Patchset 6 (Latest):
Siye Liu . resolved

Thank you!

File third_party/blink/renderer/core/html/forms/input_type.cc
Line 1239, Patchset 6 (Latest): if (DispatchBeforeInputInsertText(
Siye Liu . unresolved

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

Line 1241, Patchset 6 (Latest): current.ToString()) != DispatchEventResult::kNotCanceled) {
Siye Liu . unresolved

This is the current value. I believe we want to put the new value in `beforeinput` event.

File third_party/blink/renderer/core/html/forms/text_field_input_type.cc
Line 251, Patchset 6 (Latest): if (DispatchBeforeInputInsertText(event.target()->ToNode(),
Siye Liu . unresolved

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.

Line 252, Patchset 6 (Latest): event.KeyEvent()->text) !=
Siye Liu . unresolved

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.

Open in Gerrit

Related details

Attention is currently required from:
  • Utkarsh Pathak
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 6
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Comment-Date: Wed, 17 Apr 2024 15:38:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Utkarsh Pathak (Gerrit)

unread,
Apr 18, 2024, 5:08:17 AMApr 18
to Anupam Snigdha, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Anupam Snigdha, Rakesh Goulikar, Sanket Joshi and Siye Liu

Utkarsh Pathak added 4 comments

File third_party/blink/renderer/core/html/forms/input_type.cc
Line 1241, Patchset 6: current.ToString()) != DispatchEventResult::kNotCanceled) {
Siye Liu . resolved

This is the current value. I believe we want to put the new value in `beforeinput` event.

Utkarsh Pathak

Moved it to different method.

File third_party/blink/renderer/core/html/forms/text_field_input_type.cc
Line 251, Patchset 6: if (DispatchBeforeInputInsertText(event.target()->ToNode(),
Siye Liu . resolved

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.

Utkarsh Pathak

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.

Line 252, Patchset 6: event.KeyEvent()->text) !=
Siye Liu . unresolved

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.

Utkarsh Pathak

'''arrowup'''

Line 252, Patchset 6: event.KeyEvent()->text) !=
Siye Liu . resolved

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.

Utkarsh Pathak

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.

Open in Gerrit

Related details

Attention is currently required from:
  • Anupam Snigdha
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 7
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Thu, 18 Apr 2024 09:08:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Siye Liu <si...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
Apr 18, 2024, 12:22:23 PMApr 18
to Utkarsh Pathak, Anupam Snigdha, Sanket Joshi, Rakesh Goulikar, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Anupam Snigdha, Rakesh Goulikar, Sanket Joshi and Utkarsh Pathak

Siye Liu added 2 comments

File third_party/blink/renderer/core/html/forms/input_type.cc
Line 1126, Patchset 8 (Latest): if (DispatchBeforeInputInsertText(
Siye Liu . unresolved

Since we move the logic to `InputType::ApplyStep`, please double check if all the callers of this method are expecting the beforeinput event.

Line 1126, Patchset 8 (Latest): if (DispatchBeforeInputInsertText(
EventTargetNodeForDocument(&GetElement().GetDocument()),
new_value.ToString()) != DispatchEventResult::kNotCanceled) {
return;
}
Siye Liu . unresolved

The code looks reasonable. Can we move the code down to `// 12....`?

Open in Gerrit

Related details

Attention is currently required from:
  • Anupam Snigdha
  • Rakesh Goulikar
  • Sanket Joshi
  • Utkarsh Pathak
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 8
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Comment-Date: Thu, 18 Apr 2024 16:22:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Anupam Snigdha (Gerrit)

unread,
Apr 18, 2024, 7:15:06 PMApr 18
to Utkarsh Pathak, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sanket Joshi and Utkarsh Pathak

Anupam Snigdha added 1 comment

Commit Message
Line 7, Patchset 8 (Latest):Dispatch OnBeforeInputInsertText Event in case of number input text-box spin button
Anupam Snigdha . unresolved

Please 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
```

Open in Gerrit

Related details

Attention is currently required from:
Gerrit-Attention: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Comment-Date: Thu, 18 Apr 2024 23:14:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Anupam Snigdha (Gerrit)

unread,
Apr 22, 2024, 5:51:32 PMApr 22
to Utkarsh Pathak, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sanket Joshi, Siye Liu and Utkarsh Pathak

Anupam Snigdha added 2 comments

File third_party/blink/renderer/core/html/forms/input_type.cc
Line 1131, Patchset 9 (Latest): DispatchBeforeInputInsertText(
Anupam Snigdha . unresolved

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?

Line 1239, Patchset 6: if (DispatchBeforeInputInsertText(
Siye Liu . unresolved

Consider adding test cases for both scenarios. We want to test that the beforeinput event is fired after invoking input element event handler.

Anupam Snigdha

+1.

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
  • Utkarsh Pathak
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 9
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Comment-Date: Mon, 22 Apr 2024 21:51:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Siye Liu <si...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Utkarsh Pathak (Gerrit)

unread,
May 6, 2024, 2:28:57 AM (12 days ago) May 6
to Anupam Snigdha, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Anupam Snigdha, Rakesh Goulikar, Sanket Joshi and Siye Liu

Utkarsh Pathak added 6 comments

Commit Message
Line 7, Patchset 8:Dispatch OnBeforeInputInsertText Event in case of number input text-box spin button
Anupam Snigdha . resolved

Please 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
```

Utkarsh Pathak

Sure, I have gone through it, it was of great help and I would try to accommodate these practices in future.

File third_party/blink/renderer/core/html/forms/input_type.cc
Line 1126, Patchset 8: if (DispatchBeforeInputInsertText(
Siye Liu . resolved

Since we move the logic to `InputType::ApplyStep`, please double check if all the callers of this method are expecting the beforeinput event.

Utkarsh Pathak

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.

Line 1126, Patchset 8: if (DispatchBeforeInputInsertText(

EventTargetNodeForDocument(&GetElement().GetDocument()),
new_value.ToString()) != DispatchEventResult::kNotCanceled) {
return;
}
Siye Liu . resolved

The code looks reasonable. Can we move the code down to `// 12....`?

Utkarsh Pathak

Moved.

Line 1131, Patchset 9: DispatchBeforeInputInsertText(
Anupam Snigdha . unresolved

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?

Utkarsh Pathak

I don't see situations when the ExecutionContext will be deleted, can you share a few examples where ExecutionContext gets deleted by the JS?

Line 1239, Patchset 6: if (DispatchBeforeInputInsertText(
Siye Liu . unresolved

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

Anupam Snigdha

+1.

Utkarsh Pathak

Added Testcase for Keyboard event case and working mouse case.

File third_party/blink/renderer/core/html/forms/text_field_input_type.cc
Line 252, Patchset 6: event.KeyEvent()->text) !=
Siye Liu . resolved

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.

Utkarsh Pathak

'''arrowup'''

Utkarsh Pathak

Passing the correct number in text field to the dipatcher method.

Open in Gerrit

Related details

Attention is currently required from:
  • Anupam Snigdha
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 10
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Mon, 06 May 2024 06:28:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anupam Snigdha <sni...@microsoft.com>
Comment-In-Reply-To: Siye Liu <si...@microsoft.com>
Comment-In-Reply-To: Utkarsh Pathak <utpa...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Anupam Snigdha (Gerrit)

unread,
May 6, 2024, 12:41:59 PM (12 days ago) May 6
to Utkarsh Pathak, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sanket Joshi, Siye Liu and Utkarsh Pathak

Anupam Snigdha added 1 comment

File third_party/blink/renderer/core/html/forms/input_type.cc
Line 1131, Patchset 9: DispatchBeforeInputInsertText(
Anupam Snigdha . unresolved

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?

Utkarsh Pathak

I don't see situations when the ExecutionContext will be deleted, can you share a few examples where ExecutionContext gets deleted by the JS?

Anupam Snigdha

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();
```
Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
  • Utkarsh Pathak
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 10
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Comment-Date: Mon, 06 May 2024 16:41:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anupam Snigdha <sni...@microsoft.com>
Comment-In-Reply-To: Utkarsh Pathak <utpa...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
May 7, 2024, 1:49:35 PM (11 days ago) May 7
to Utkarsh Pathak, Anupam Snigdha, Sanket Joshi, Rakesh Goulikar, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sanket Joshi and Utkarsh Pathak

Siye Liu added 3 comments

File third_party/blink/renderer/core/html/forms/html_input_element_test.cc
Line 232, Patchset 12 (Latest): // crbug.com/40948436
Siye Liu . unresolved

Nit: Move the comment outside of the test case:

```
// crbug.com/40948436
TEST_F(HTMLInputElementTest,
NumberInputOnBeforeInputEventFiringForKeyBoardEvent) {
```
Line 242, Patchset 12 (Latest): "document.getElementById('ele').addEventListener('beforeinput', "
Siye Liu . unresolved

What happened if the event is prevent defaulted? Can you add test case for that?

Line 252, Patchset 12 (Latest): auto& keyboard_event =
Siye Liu . unresolved

Nit: `auto` is used here but real type is used elsewhere. Please consider using either way consistently.

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sanket Joshi
  • Utkarsh Pathak
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 12
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Comment-Date: Tue, 07 May 2024 17:49:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Anupam Snigdha (Gerrit)

unread,
May 7, 2024, 5:35:38 PM (10 days ago) May 7
to Utkarsh Pathak, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sanket Joshi and Utkarsh Pathak

Anupam Snigdha added 1 comment

File third_party/blink/renderer/core/html/forms/input_type.cc
Line 1138, Patchset 12 (Latest): DispatchBeforeInputInsertText(
Anupam Snigdha . unresolved

Can we add a WPT test as well to verify that `beforeinput` event is dispatched correctly for this case?

Gerrit-Comment-Date: Tue, 07 May 2024 21:35:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
May 9, 2024, 1:05:43 PM (9 days ago) May 9
to Utkarsh Pathak, Anupam Snigdha, Sanket Joshi, Rakesh Goulikar, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Anupam Snigdha, Rakesh Goulikar, Sanket Joshi and Utkarsh Pathak

Siye Liu added 7 comments

File third_party/blink/renderer/core/html/forms/html_input_element_test.cc

Nit: Move the comment outside of the test case:

```
// crbug.com/40948436
TEST_F(HTMLInputElementTest,
NumberInputOnBeforeInputEventFiringForKeyBoardEvent) {
```
Siye Liu

Done

Line 236, Patchset 13 (Latest): "<div id='editableText'></div>");
Siye Liu . unresolved

Nit: Even thouch the name is "editableText", the div is non-contenteditable. Would be better to rename it to something like "log".

Line 242, Patchset 12: "document.getElementById('ele').addEventListener('beforeinput', "
Siye Liu . resolved

What happened if the event is prevent defaulted? Can you add test case for that?

Siye Liu

Done

Line 252, Patchset 12: auto& keyboard_event =
Siye Liu . resolved

Nit: `auto` is used here but real type is used elsewhere. Please consider using either way consistently.

Siye Liu

Done

Line 288, Patchset 13 (Latest): number_input.Focus();
Siye Liu . unresolved

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.

Line 295, Patchset 13 (Latest): EXPECT_NE(1, number_input.valueAsNumber());
Siye Liu . unresolved

Should this be 0?

File third_party/blink/renderer/core/html/forms/input_type.cc
Line 1239, Patchset 6: if (DispatchBeforeInputInsertText(
Siye Liu . resolved

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

Anupam Snigdha

+1.

Utkarsh Pathak

Added Testcase for Keyboard event case and working mouse case.

Siye Liu

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Anupam Snigdha
  • Rakesh Goulikar
  • Sanket Joshi
  • Utkarsh Pathak
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 13
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Comment-Date: Thu, 09 May 2024 17:05:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anupam Snigdha <sni...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Utkarsh Pathak (Gerrit)

unread,
May 10, 2024, 5:18:28 AM (8 days ago) May 10
to Anupam Snigdha, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Anupam Snigdha, Rakesh Goulikar, Sanket Joshi and Siye Liu

Utkarsh Pathak added 4 comments

Patchset-level comments
File-level comment, Patchset 15 (Latest):
Utkarsh Pathak . resolved

Resolving Siye Liu's comments.

File third_party/blink/renderer/core/html/forms/html_input_element_test.cc
Line 236, Patchset 13: "<div id='editableText'></div>");
Siye Liu . resolved

Nit: Even thouch the name is "editableText", the div is non-contenteditable. Would be better to rename it to something like "log".

Utkarsh Pathak

Makes sense, changed the ID to"log"

Line 288, Patchset 13: number_input.Focus();
Siye Liu . resolved

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.

Utkarsh Pathak

Added this value check in all 4 TCs

Line 295, Patchset 13: EXPECT_NE(1, number_input.valueAsNumber());
Siye Liu . resolved

Should this be 0?

Utkarsh Pathak

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

Open in Gerrit

Related details

Attention is currently required from:
  • Anupam Snigdha
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 15
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Fri, 10 May 2024 09:18:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Siye Liu <si...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
May 13, 2024, 12:18:02 PM (5 days ago) May 13
to Utkarsh Pathak, Anupam Snigdha, Sanket Joshi, Rakesh Goulikar, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Anupam Snigdha, Rakesh Goulikar, Sanket Joshi and Utkarsh Pathak

Siye Liu added 1 comment

File third_party/blink/renderer/core/html/forms/input_type.cc
Line 1131, Patchset 9: DispatchBeforeInputInsertText(
Anupam Snigdha . unresolved

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?

Utkarsh Pathak

I don't see situations when the ExecutionContext will be deleted, can you share a few examples where ExecutionContext gets deleted by the JS?

Anupam Snigdha

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();
```
Siye Liu

+1

Open in Gerrit

Related details

Attention is currently required from:
  • Anupam Snigdha
  • Rakesh Goulikar
  • Sanket Joshi
  • Utkarsh Pathak
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 15
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Comment-Date: Mon, 13 May 2024 16:17:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Anupam Snigdha (Gerrit)

unread,
May 16, 2024, 2:05:03 PM (2 days ago) May 16
to Utkarsh Pathak, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sanket Joshi, Siye Liu and Utkarsh Pathak

Anupam Snigdha added 4 comments

Commit Message
Line 9, Patchset 16 (Latest):
Anupam Snigdha . unresolved

Can you add a description of this change?

File third_party/blink/renderer/core/html/forms/html_input_element_test.cc
Line 243, Patchset 16 (Latest): "document.getElementById('ele').addEventListener('beforeinput', "
" () => "
"document.getElementById('log').textContent='beforeinput event "
"was fired!');");
Anupam Snigdha . unresolved

Can we add this scenario as a web test/WPT?

File third_party/blink/renderer/core/html/forms/input_type.cc
Line 1131, Patchset 9: DispatchBeforeInputInsertText(
Anupam Snigdha . resolved

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?

Utkarsh Pathak

I don't see situations when the ExecutionContext will be deleted, can you share a few examples where ExecutionContext gets deleted by the JS?

Anupam Snigdha

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();
```
Siye Liu

+1

Anupam Snigdha

Done

Line 1138, Patchset 12: DispatchBeforeInputInsertText(
Anupam Snigdha . resolved

Can we add a WPT test as well to verify that `beforeinput` event is dispatched correctly for this case?

Anupam Snigdha

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
  • Utkarsh Pathak
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 16
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Comment-Date: Thu, 16 May 2024 18:04:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anupam Snigdha <sni...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
May 16, 2024, 2:07:31 PM (2 days ago) May 16
to Utkarsh Pathak, Anupam Snigdha, Sanket Joshi, Rakesh Goulikar, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sanket Joshi and Utkarsh Pathak

Siye Liu added 2 comments

File third_party/blink/web_tests/fast/forms/number/number-input-keyboardevent.html
Line 1, Patchset 16 (Latest):<!DOCTYPE html>
Siye Liu . unresolved

Nit: Consider renaming this file name with more details about the test case.

Line 22, Patchset 16 (Latest):eventSender.keyDown('ArrowUp');
Siye Liu . unresolved

It'd be good to verify that `inputElement` and `child` are gone after this line.

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sanket Joshi
  • Utkarsh Pathak
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Idc894f4e7427a81e684b37cc10749b4386e5aa7d
Gerrit-Change-Number: 5443103
Gerrit-PatchSet: 16
Gerrit-Owner: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Reviewer: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-CC: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Utkarsh Pathak <utpa...@microsoft.com>
Gerrit-Comment-Date: Thu, 16 May 2024 18:07:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages