[SVG] Add window-reflecting event handlers to SVGSVGElement per SVG2 spec [chromium/src : main]
Vinay Singh (Gerrit)
Vinay Singh voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vinay Singh (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Divyansh Mangal
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 22 comments![]( "Open in Gerrit")
Since there are changes to an IDL this will require an I2S as well, please create an entry in the Chrome Status Entry and add the link here or in the description
WindowEventHandlers mixin includes more than just these 6 events. The CL focuses on the ones that overlap with `GlobalEventHandlers1 (which need explicit shadowing). This is the right prioritization, but a follow-up CL for the remaining WindowEventHandlers
attributes should be tracked.
The SVG2 spec requires SVGSVGElement to forward certain event handler
IDL attributes (onblur, onerror, onfocus, onload, onresize, onscroll) to
the window object — matching HTMLBodyElement behavior.
Previously only inline content attributes (e.g., <svg onresize="...">)
were forwarded via ParseAttribute(). The IDL property path
(svgElement.onresize = fn) went through GlobalEventHandlers and set the
listener on the SVG element itself, not the window.
This CL:
- Adds CORE_EXPORT to SVGSVGElement for cross-component visibility
- Adds WindowEventHandlers C++ mixin to SVGSVGElement class
- Shadows the 6 GlobalEventHandlers events that overlap with
window-reflecting behavior (blur, error, focus, load, resize,
scroll) using DEFINE_WINDOW_ATTRIBUTE_EVENT_LISTENER macros
- Adds explicit IDL attribute overrides for these 6 events
- Extends ParseAttribute to forward onblur, onfocus content
attributes to the window for the outermost <svg> element
- Adds WPT tests for the new behavior
- Updates webexposed test baselinesConsider using quotes(`) for code/element/event to separate them from normal text. There are atleast 15+ such references in this description.
GetExecutionContext(), name, value));
} else if (name == html_names::kOnblurAttr) {
GetDocument().SetWindowAttributeEventListener(
event_type_names::kBlur, JSEventHandlerForContentAttribute::Create(
GetExecutionContext(), name, value));
} else if (name == html_names::kOnfocusAttr) {
GetDocument().SetWindowAttributeEventListener(
event_type_names::kFocus, JSEventHandlerForContentAttribute::Create(
GetExecutionContext(), name, value));Are you missing some attributes here like `html_names::kOnLoad` ?
// https://svgwg.org/svg2-draft/struct.html#InterfaceSVGSVGElementnit: link is already given at the top of the interface no need to include it again.
attribute EventHandler onblur;
attribute OnErrorEventHandler onerror;
attribute EventHandler onfocus;
attribute EventHandler onload;
attribute EventHandler onresize;
attribute EventHandler onscroll;On that note, I don't see attributes like these on the IDL at all, infact the idl mentions the mixin like this
`SVGSVGElement includes WindowEventHandlers;`
The corresponding html_body_element.idl has them but there is a todo for that as well. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_body_element.idl;drc=e207afe9704990b39825430e89912140cfee2b84;l=35
From my finding Gecko does not support these attributes in their IDL, but they also don't have the mixin `WindowEventHandlers` in their IDL so I am assuming it's not supported there currently.
But their `HTMLBodyElement` IDL does have the mixin without these attributes
https://searchfox.org/firefox-main/source/dom/webidl/HTMLBodyElement.webidl
I think the line `SVGSVGElement includes WindowEventHandlers;` should be added but should the attributes be added like this as you did in this CL?
attribute EventHandler onblur;
attribute OnErrorEventHandler onerror;
attribute EventHandler onfocus;
attribute EventHandler onload;
attribute EventHandler onresize;
attribute EventHandler onscroll;if at all we even need them these should be behind runtime feature flag
`[RuntimeEnabled=]`
<link rel="help" href="https://svgwg.org/svg2-draft/struct.html#InterfaceSVGSVGElement">I think you are missing a test for the `onload` here too.
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>You can add an author link like:
`<link rel="author" title="Divyansh Mangal" href="mailto:dma...@microsoft.com">`
var svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");nit: can this be made `const` or `let`
var svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");nit: can this be made `const` or `let`
var svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");nit: can this be made `const` or `let`
var handler = function() {};nit: can this be made `const` or `let`
var previousHandler = window.onerror;nit: can this be made `const` or `let`
var svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");nit: can this be made `const` or `let`
var svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");nit: can this be made `const` or `let`
var svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");nit: can this be made `const` or `let`
var svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");nit: can this be made `const` or `let`
var handler = function() {};nit: can this be made `const` or `let`
var previousHandler = window.onresize;nit: can this be made `const` or `let`
<!DOCTYPE html>do we need this custom test at all? doesn't the WPT already covers forwarding a bit better?
Related details
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vinay Singh (Gerrit)
Vinay Singh added 22 comments![]( "Open in Gerrit")
Agreed, good call. Added a follow-up tracking note in the CL description to track the remaining `WindowEventHandlers` attributes (`onafterprint`, `onbeforeprint`, `onhashchange`, `onmessage`, etc.) in a subsequent CL.
Acknowledged. Will create a Chrome Status entry for the I2S as a follow-up action item before landing.
Done. Added backtick formatting for all code/element/event references throughout the description.
Good catch! Added `html_names::kOnloadAttr` forwarding in `ParseAttribute()` for the outermost `<svg>` element, matching `HTMLBodyElement` behavior.
The explicit IDL attributes are intentional and necessary here. `SVGElement` includes `GlobalEventHandlers`, which already defines `onblur`, `onerror`, `onfocus`, `onload`, `onresize`, `onscroll` with element-local semantics. Without explicit overrides in the IDL, the inherited `GlobalEventHandlers` versions would be used, setting handlers on the element itself rather than forwarding to the window.
This is the exact same pattern `HTMLBodyElement` uses (see html_body_element.idl) - it also has explicit attribute overrides for these 6 events alongside `HTMLBodyElement includes WindowEventHandlers;`.
Adding `SVGSVGElement includes WindowEventHandlers;` alone would only add the *non-overlapping* `WindowEventHandlers` events (`onafterprint`, `onbeforeprint`, etc.) but would NOT handle the shadowing of the 6 `GlobalEventHandlers` events. That mixin line is planned for the follow-up CL.
`HTMLBodyElement` has the equivalent explicit attributes without a `[RuntimeEnabled]` flag. This implements spec-mandated behavior that has existed for `<body>` since the beginning. Adding a flag here would create an inconsistency with `HTMLBodyElement` and diverge from the spec for no clear benefit. The behavior for content attributes (`<svg onresize='...'>`) already works without a flag - this CL just fixes the IDL property path to match.
Done. Added an `onload` test using property reflection (same approach as the `onerror` test, to avoid interfering with testharness.js).
Yes, `<div id='log'>` is the standard WPT testharness.js convention for rendering test results. While testharness.js will create one if missing, including it explicitly is the recommended pattern per WPT documentation.
Done. Changed all `var` declarations to `const` (or `let` where reassignment occurs) throughout the test.
Related details
- Divyansh Mangal
- Fredrik Söderquist
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 5 comments![]( "Open in Gerrit")
Acknowledged. Will create a Chrome Status entry for the I2S as a follow-up action item before landing.
You can create a Chrome Status entry as a placeholder as well. The details can be filled later, you only need the link here.
Agreed, good call. Added a follow-up tracking note in the CL description to track the remaining `WindowEventHandlers` attributes (`onafterprint`, `onbeforeprint`, `onhashchange`, `onmessage`, etc.) in a subsequent CL.
Please add a TODO for this before closing this.
Done. Added backtick formatting for all code/element/event references throughout the description.
You say its done, but I still see many references without the backtick (`). Please check again
`HTMLBodyElement` has the equivalent explicit attributes without a `[RuntimeEnabled]` flag. This implements spec-mandated behavior that has existed for `<body>` since the beginning. Adding a flag here would create an inconsistency with `HTMLBodyElement` and diverge from the spec for no clear benefit. The behavior for content attributes (`<svg onresize='...'>`) already works without a flag - this CL just fixes the IDL property path to match.
But this is a new API change specific to `SVGSVGElement` we generally start with Runtime feature flag where IDL is getting changed.
The explicit IDL attributes are intentional and necessary here. `SVGElement` includes `GlobalEventHandlers`, which already defines `onblur`, `onerror`, `onfocus`, `onload`, `onresize`, `onscroll` with element-local semantics. Without explicit overrides in the IDL, the inherited `GlobalEventHandlers` versions would be used, setting handlers on the element itself rather than forwarding to the window.
This is the exact same pattern `HTMLBodyElement` uses (see html_body_element.idl) - it also has explicit attribute overrides for these 6 events alongside `HTMLBodyElement includes WindowEventHandlers;`.
Adding `SVGSVGElement includes WindowEventHandlers;` alone would only add the *non-overlapping* `WindowEventHandlers` events (`onafterprint`, `onbeforeprint`, etc.) but would NOT handle the shadowing of the 6 `GlobalEventHandlers` events. That mixin line is planned for the follow-up CL.
Please add the TODO before closing this, there is a similar TODO in `HTMLBodyElement` as well
Related details
- Fredrik Söderquist
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vinay Singh (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Divyansh Mangal
- Fredrik Söderquist
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 14 comments![]( "Open in Gerrit")
DEFINE_WINDOW_ATTRIBUTE_EVENT_LISTENER(resize, kResize)
DEFINE_WINDOW_ATTRIBUTE_EVENT_LISTENER(scroll, kScroll)So, the content attribute processing for these only apply if the element is outermost, while here it always apply.
DEFINE_WINDOW_ATTRIBUTE_EVENT_LISTENER(blur, kBlur)
DEFINE_WINDOW_ATTRIBUTE_EVENT_LISTENER(error, kError)
DEFINE_WINDOW_ATTRIBUTE_EVENT_LISTENER(focus, kFocus)
DEFINE_WINDOW_ATTRIBUTE_EVENT_LISTENER(load, kLoad)Some of these are not handled the same as content attributes.
// WindowEventHandlers overrides for events that also exist in
// GlobalEventHandlers (same pattern as HTMLBodyElement).I think this might be more helpful if it said why. (Also, `WindowEventHandlers` not really relevant (yet).)
public WindowEventHandlers {We're not really exposing any of these though, so at this point, this is jut dead weight. You can defined `GetDocumentForWindowEventHandler()` without overriding it.
Divyansh Mangal`HTMLBodyElement` has the equivalent explicit attributes without a `[RuntimeEnabled]` flag. This implements spec-mandated behavior that has existed for `<body>` since the beginning. Adding a flag here would create an inconsistency with `HTMLBodyElement` and diverge from the spec for no clear benefit. The behavior for content attributes (`<svg onresize='...'>`) already works without a flag - this CL just fixes the IDL property path to match.
But this is a new API change specific to `SVGSVGElement` we generally start with Runtime feature flag where IDL is getting changed.
👍
Are the content attribute versions tested somewhere? If not, it might make sense to do that here as well?
const svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");
document.body.appendChild(svg);
this.add_cleanup(function() { svg.remove(); });
// SVGSVGElement should have the window-reflecting event handler attributes.
assert_true('onblur' in svg,
"SVGSVGElement should have onblur");
assert_true('onerror' in svg,
"SVGSVGElement should have onerror");
assert_true('onfocus' in svg,
"SVGSVGElement should have onfocus");
assert_true('onload' in svg,
"SVGSVGElement should have onload");
assert_true('onresize' in svg,
"SVGSVGElement should have onresize");
assert_true('onscroll' in svg,
"SVGSVGElement should have onscroll");This looks like it may be better suited to an IDL harness test? Reagrdless there should be no need to create an instance, and testharness has better functions for this style of test.
svg.onresize = t.step_func(function(e) {`step_func_done()` (and drop the `t.done()`).
async_test(function(t) {
const svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");
document.body.appendChild(svg);
t.add_cleanup(function() { svg.remove(); });
svg.onresize = t.step_func(function(e) {
assert_equals(e.currentTarget, window,
"resize event dispatched on window should have currentTarget === window");
t.done();
});
window.dispatchEvent(new Event('resize'));
}, "Setting SVGSVGElement.onresize should forward the handler to the window");It looks like you could write this test once and parameterize it on the event name.
window.onerror = previousHandler;I guess you want this to be part of the cleanup as well?
// Test: addEventListener('resize', ...) on SVGSVGElement does NOT forward to
// the window. Only IDL attribute (onresize) and content attribute
// (<svg onresize="...">) forwarding is specified.This looks like something that should hold true in general for the events tested? Why limit to `resize`?
async_test(function(t) {Does this test need to be async? The way it's written currently it sort of assumes synchronicity.
window.addEventListener('resize', function handler(e) {This looks like it should `unreachable_func()`?
window.removeEventListener('resize', handler);Use `{once: true}`, but we don't expect it to happen, so should probably just clean it up unconditionally at the end of the test.
Related details
- Divyansh Mangal
- Vinay Singh
- Virali Purbey