search

w3c / selection-api

Selection API
http://w3c.github.io/selection-api/
Other
47 stars 29 forks source link

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

Closed ShuangshuangZhou closed 7 months ago

ShuangshuangZhou commented 9 months ago

Hello!

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

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

What is the script observable behavior difference?

ShuangshuangZhou commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago
  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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

ShuangshuangZhou commented 8 months ago

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 commented 8 months ago

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 commented 7 months ago

Hm... this issue interacts with https://github.com/w3c/selection-api/issues/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.

ShuangshuangZhou commented 7 months ago

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 commented 7 months ago

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

rniwa commented 7 months ago

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: