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

Skip posting a task for selectionchange event in some cases #170

Closed
ShuangshuangZhou opened this issue Feb 1, 2024 · 15 comments
Closed

Comments

@ShuangshuangZhou
Copy link

Hello!

We may consider to skip posting a task for the selectionchange event in some specific cases like:

  • Skip posting a task for selectionchange event if there are no registered listeners;
  • Skip posting a task for selectionchange event if the UA already has an queued task for an equivalent selectionchange event;

Here is a simple case when we don't have any registered listener:

<!DOCTYPE html>
<html>
<form>
    <div>
        <input
        class="toggle-all"
        type="text"
        id="inputtext"
        name="subscribe"
        value="hello world"/>
    </div>
  </form>

  <script>
  var tx = document.getElementById("inputtext");
  tx.focus();
  for (var i = 0; i < 100; i++) {

    console.time("set value");
    tx.value = 'something to do ' + i;
    console.timeEnd("set value");
  }
  </script>
  </html>

Based the below chrome trace, it is noticed that although we don't register any listener, it still posts an async task when we are just changing the text value.
selectionchange-for-w3c

@tkent-google
Copy link

@rniwa What do you think about this optimization? Chromium is positive about it.

It's a web-exposed change. But I guess it won't cause compatibility issues.

@ShuangshuangZhou
Copy link
Author

ShuangshuangZhou commented Feb 21, 2024

Hi@rniwa Sorry to disturb, any comments on this?
Besides in Chromium, I take a look in WebCore, seems that WebCore also enqueue events(link) without checking whether the event has related listeners. Speedometer2.1/3 might get some progression when skipping these events w/o listeners.

Thanks!

@rniwa
Copy link
Contributor

rniwa commented Feb 21, 2024

What is the script observable behavior difference?

@ShuangshuangZhou
Copy link
Author

On web user side, it has no observable behavior difference because currently we haven't registered any listeners(in the listed script).

Skip enqueueing and dispatching this event without listeners might have some benefits on performance, since the browser doesn't need to dispatch events and excute related task and save some CPU cycles.

@rniwa
Copy link
Contributor

rniwa commented Feb 21, 2024

If there is no script observable behavior difference, I don't think we need to say one way or another in the spec.

@tkent-google
Copy link

  1. There are no event listeners for selectionchange
  2. input.selectionEnd = 42 Update selection, and post a task to dispatch selectionchagne
  3. addEventListener('selectionchange', callback)
  4. Enter the event loop
  5. The task is executed, and the callback is called.

The callback won't be called if UAs skip to post the task.


  1. addEventListener('selectionchange', callback)
  2. input.selectionEnd = 42 Update selection, and post a task to dispatch selectionchagne
  3. input.selectionEnd = 41 Update selection, and post another task to dispatch selectionchagne
  4. Enter the event loop
  5. The tasks are executed, and the callback is called twice.

The callback will be called only once if UAs skip to post the second task.

@rniwa
Copy link
Contributor

rniwa commented Feb 21, 2024

I see. That sounds like something that could potentially pose a web compatibility problem to me. Also, generally speaking, we don't want to change the UA behavior based on whether an event listener is present or not. So that makes me think that we probably don't want to avoid posting a task even when there is no event listener but perhaps we can add a flag to check whether a task to fire selectionchange had already been scheduled or not.

@ShuangshuangZhou
Copy link
Author

ShuangshuangZhou commented Feb 21, 2024

I see. Maybe the logic here is something like below(flag selection_change_event_scheduled is set to false by default)?

if (!document.selection_change_event_scheduled) {
  post_selection_change_task();
  document.selection_change_event_scheduled = true;
  ...
}
 
.....
 
selection_change_task_callback(){
  ...
  document.selection_change_event_scheduled = false;
}

@rniwa
Copy link
Contributor

rniwa commented Feb 21, 2024

Well, you need to reset the flag at the beginning of the callback, not at the end, but yeah.

@ShuangshuangZhou
Copy link
Author

ShuangshuangZhou commented Feb 22, 2024

Got it. Thx for the correction. So officially, we need to write this into the selectionchange-event spec, right? What is the subsequent process like?

Thanks!

@annevk
Copy link
Member

annevk commented Feb 22, 2024

Yeah, a PR against this repository as well as a PR against web-platform-tests for test coverage. And ideally some indication that multiple implementers are on board.

rniwa added a commit that referenced this issue Mar 15, 2024
Add a boolean flag "has scheduled selectionchange event" to selection,
and check it before queuing a task to fire a selectionchange event.
@rniwa
Copy link
Contributor

rniwa commented Mar 15, 2024

Hm... this issue interacts with #53. The spec currently specifies Firefox's behavior of firing selectionchange event at input and textarea elements. However, this is problematic with respect to the new boolean state we're considering to introduce in this issue. It would be odd for the boolean state to be document-wide if we kept the current model since selectionchange maybe scheduled to be fired on different input elements. An obvious alternative is to have a boolean state per each input / textarea element. Either way, we need to figure out which behavior is most web compatible & makes most sense.

rniwa added a commit that referenced this issue Mar 15, 2024
Add a boolean flag "has scheduled selectionchange event" to document,
input element, and textarea element, and check it before queuing a task
to fire a selectionchange event.
@ShuangshuangZhou
Copy link
Author

ShuangshuangZhou commented Mar 15, 2024

Hi Rniwa, thanks for the sharing! I've read issue#53 and basically understand what you guys want to do. You'd like to fire selectionchange event for each element(input or textarea) when it is changed no matter the element is focused or not, right?

I take a look on the definition of selection in the spec, seems that one document only have one unique selection, so only the change on the focused element should fire the event, right? So why the spec keeps the same? Is there anything blocked what we want to do in issue#53?

Please corret me if my understanding is wrong. Thanks!

rniwa added a commit that referenced this issue Mar 16, 2024
Add a boolean flag "has scheduled selectionchange event" to document,
input element, and textarea element, and check it before queuing a task
to fire a selectionchange event.
rniwa added a commit to rniwa/web-platform-tests that referenced this issue Mar 16, 2024
@rniwa
Copy link
Contributor

rniwa commented Mar 16, 2024

So now we have a PR for spec change and a PR for WPT.

rniwa added a commit to rniwa/web-platform-tests that referenced this issue Mar 16, 2024
rniwa added a commit that referenced this issue Mar 16, 2024
Add a boolean flag "has scheduled selectionchange event" to document,
input element, and textarea element, and check it before queuing a task
to fire a selectionchange event.
@rniwa
Copy link
Contributor

rniwa commented Mar 16, 2024

So my proposal as the editor of a spec is as follows. We introduce boolean state has scheduled selectionchange event on document, input element, and textarea element. We introduce two algorithms:

  • To schedule a selectionchange event on a node target - If the boolean state of target is false, this algorithm queues a task to fire a selectionchange event and sets the boolean state of target to true. Otherwise, if the boolean state of target is true, it does nothing.
  • To fire a selectionchange event on a node target - Sets the boolean state of target to false and fires selectionchange event on target.

@rniwa rniwa closed this as completed in 4397d66 Mar 20, 2024
rniwa added a commit to rniwa/web-platform-tests that referenced this issue Mar 20, 2024
rniwa added a commit to rniwa/web-platform-tests that referenced this issue Mar 20, 2024
rniwa added a commit to web-platform-tests/wpt that referenced this issue Mar 22, 2024
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 28, 2024
…selection-api#170, a=testonly

Automatic update from web-platform-tests
Coalesce selectionchange events per w3c/selection-api#170 (#45145)

--

wpt-commits: fd3510de36638b07103738b9ff2bfa9ede340797
wpt-pr: 45145
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Apr 1, 2024
…selection-api#170, a=testonly

Automatic update from web-platform-tests
Coalesce selectionchange events per w3c/selection-api#170 (#45145)

--

wpt-commits: fd3510de36638b07103738b9ff2bfa9ede340797
wpt-pr: 45145
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants