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

Fixes https://github.com/w3c/selection-api/issues/170. #172

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

rniwa
Copy link
Contributor

@rniwa rniwa commented 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.


Preview | Diff

@rniwa rniwa force-pushed the schedule-selectionchange-once branch from 985c48d to c3903d0 Compare March 15, 2024 03:00
index.html Show resolved Hide resolved
abort these steps.
</li>
<li>
<a>Queue a task</a> on the <a>user interaction task source</a> to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of queue a task, you want https://html.spec.whatwg.org/#queue-an-element-task and also pass that target. This way some aspects of the task get better initialized.

(We want to eventually remove "queue a task".)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that'll work because target can be a document, which isn't an element.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I guess we can invoke https://html.spec.whatwg.org/#queue-a-global-task instead and obtain the global ourselves, but it seems there should be a helper for that. Maybe the primitive should be "queue an object task" where "object" has to be a "platform object". @domenic what do you think about that?

I also wonder if someone moves an element from one document to another, should we really have the global from the former document or should we use the element's node document's global object?

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

So currently we don't do this for the only focused element, right? For example, for several input elements on the same page, every input has its own has_scheduled_selectionchange_event, and we will do same things for every input or text area .

@rniwa
Copy link
Contributor Author

rniwa commented Mar 18, 2024

So currently we don't do this for the only focused element, right? For example, for several input elements on the same page, every input has its own has_scheduled_selectionchange_event, and we will do same things for every input or text area .

Right. Boolean is per element.

webkit-commit-queue pushed a commit to rniwa/WebKit that referenced this pull request Mar 20, 2024
https://bugs.webkit.org/show_bug.cgi?id=271117

Reviewed by Chris Dumez.

Implement the spec change to avoid queuing a task to fire a selectionchange event when there is already a task scheduled
to do that for the target: w3c/selection-api#172

Also update the relevant web platform tests: web-platform-tests/wpt#45145

* LayoutTests/fast/events/selectionchange-iframe-expected.txt:
* LayoutTests/fast/events/selectionchange-user-initiated-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/selection/onselectionchange-on-distinct-text-controls-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/selection/onselectionchange-on-distinct-text-controls.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/selection/onselectionchange-on-document-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/selection/onselectionchange-on-document.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/selection/textcontrols/selectionchange.html:
* Source/WebCore/editing/FrameSelection.cpp:
(WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance):
* Source/WebCore/editing/FrameSelection.h:
* Source/WebCore/html/HTMLTextFormControlElement.cpp:
(WebCore::HTMLTextFormControlElement::HTMLTextFormControlElement):
(WebCore::HTMLTextFormControlElement::scheduleSelectionChangeEvent):
(WebCore::HTMLTextFormControlElement::scheduleSelectEvent):
* Source/WebCore/html/HTMLTextFormControlElement.h:
(WebCore::HTMLTextFormControlElement::hasScheduledSelectionChangeEvent const):
(WebCore::HTMLTextFormControlElement::setHasScheduledSelectionChangeEvent):

Canonical link: https://commits.webkit.org/276388@main
@rniwa
Copy link
Contributor Author

rniwa commented Mar 20, 2024

WebKit has implemented this new behavior as of https://commits.webkit.org/276388@main.

@rniwa rniwa merged commit 4397d66 into gh-pages Mar 20, 2024
2 checks passed
@rniwa rniwa deleted the schedule-selectionchange-once branch March 20, 2024 06:07
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 18, 2024
According to the new spec[1], we need enqueue selectionchange event
per element. WebKit also updated the implementation of selectionchange
event in webcore[2].

[1] w3c/selection-api#172
[2] https://commits.webkit.org/276238@main

Change-Id: If2020febecdf32d6165023712442f16c6e91bd4c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 22, 2024
According to the new spec[1], we need enqueue selectionchange event
per element. WebKit also updated the implementation of selectionchange
event in webcore[2].

[1] w3c/selection-api#172
[2] https://commits.webkit.org/276238@main

Change-Id: If2020febecdf32d6165023712442f16c6e91bd4c
aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 24, 2024
According to the new spec[1], we need enqueue selectionchange event
per element. WebKit also updated the implementation of selectionchange
event in webcore[2].

[1] w3c/selection-api#172
[2] https://commits.webkit.org/276238@main

Bug: 330766600
Change-Id: If2020febecdf32d6165023712442f16c6e91bd4c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5393615
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Shuangshuang Zhou <shuangshuang.zhou@intel.com>
Cr-Commit-Position: refs/heads/main@{#1291702}
@smaug----
Copy link

smaug---- commented May 22, 2024

Curious, is this (possibly breaking) change already in the shipping version of Safari? (I don't have a Mac around atm). I'm changing the behavior in Gecko. Test runs are still pending.

@rniwa
Copy link
Contributor Author

rniwa commented May 22, 2024

No, it hasn't shipped yet.

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

Successfully merging this pull request may close these issues.

None yet

4 participants