Promisify scroll: element.idl changes behind a flag. [chromium/src : main]
Mustaq Ahmed (Gerrit)
Message from Mustaq Ahmed![]( "Open in Gerrit")
Set Ready For Review
Related details
- Mustaq Ahmed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 1 comment![]( "Open in Gerrit")
ptal (the predecessor CL would be a no-change after the spec pr lands).
Related details
- Vladimir Levin
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladimir Levin (Gerrit)
Vladimir Levin added 7 comments![]( "Open in Gerrit")
void scrollIntoView(const V8UnionBooleanOrScrollIntoViewOptions* arg);It might be worthwhile to rename these to ScrollIntoViewForTesting and similar if they are only for tests (can be a follow-up)
ScriptPromise<IDLUndefined> AdHocPromiseResolvedImmediately(nit: CreateScrollResolvedPromise
ScriptPromise<IDLUndefined> PromiseRejectedWithDocumentError(similarly CreateScrollRejectedPromise
resolver->RejectWithDOMException(DOMExceptionCode::kWrongDocumentError,What happens when scroll functions are called on the wrong document today?
return AdHocPromiseResolvedImmediately(script_state);I assume there's follow-up to actually implement the timing correctly? If so, can you add a TODO here
if (!InActiveDocument()) {I don't think this is a "wrong document" but rather not an active document, so the error can likely be better. I'm also not convinced that we should error, rather than just resolving silently
[FAIL] Element interface: operation scrollIntoView(optional (boolean or ScrollIntoViewOptions))If we resolve the promise, do these go away?
Related details
- Mustaq Ahmed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 7 comments![]( "Open in Gerrit")
void scrollIntoView(const V8UnionBooleanOrScrollIntoViewOptions* arg);It might be worthwhile to rename these to ScrollIntoViewForTesting and similar if they are only for tests (can be a follow-up)
Added a TODO for now.
ScriptPromise<IDLUndefined> AdHocPromiseResolvedImmediately(Mustaq Ahmednit: CreateScrollResolvedPromise
Done
ScriptPromise<IDLUndefined> PromiseRejectedWithDocumentError(Mustaq Ahmedsimilarly CreateScrollRejectedPromise
Done
resolver->RejectWithDOMException(DOMExceptionCode::kWrongDocumentError,What happens when scroll functions are called on the wrong document today?
As you pointed out below, it is about "disconnected element" and not "wrong document". A call on a disconnected element is silently ignored.
return AdHocPromiseResolvedImmediately(script_state);I assume there's follow-up to actually implement the timing correctly? If so, can you add a TODO here
I added a TODO in the function def above, instead of repeating the same comment at all callers.
I don't think this is a "wrong document" but rather not an active document, so the error can likely be better. I'm also not convinced that we should error, rather than just resolving silently
Yeah, that would match [our spec PR](https://github.com/w3c/csswg-drafts/pull/12355) too. Removed the rejection code.
[FAIL] Element interface: operation scrollIntoView(optional (boolean or ScrollIntoViewOptions))If we resolve the promise, do these go away?
The error remains the same with "always resolve". Even the first diff above ("unhandled rejection") remains.
Related details
- Vladimir Levin
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 1 comment![]( "Open in Gerrit")
void scrollIntoView(const V8UnionBooleanOrScrollIntoViewOptions* arg);Mustaq AhmedIt might be worthwhile to rename these to ScrollIntoViewForTesting and similar if they are only for tests (can be a follow-up)
Added a TODO for now.
All are renamed in a followup CL: https://chromium-review.googlesource.com/c/chromium/src/+/6658538
Related details
- Vladimir Levin
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladimir Levin (Gerrit)
Vladimir Levin added 1 comment![]( "Open in Gerrit")
[FAIL] Element interface: operation scrollIntoView(optional (boolean or ScrollIntoViewOptions))Mustaq AhmedIf we resolve the promise, do these go away?
The error remains the same with "always resolve". Even the first diff above ("unhandled rejection") remains.
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 1 comment![]( "Open in Gerrit")
[FAIL] Element interface: operation scrollIntoView(optional (boolean or ScrollIntoViewOptions))Mustaq AhmedIf we resolve the promise, do these go away?
Vladimir LevinThe error remains the same with "always resolve". Even the first diff above ("unhandled rejection") remains.
Why is this error happening?
The idlharness test confirms that the exposed `element.scroll*` methods match the *external* spec-ed idl definitions in `external/wpt/interfaces/cssom-view.idl`. The external one would change only after my [PR](https://github.com/w3c/csswg-drafts/pull/12355) lands.
Here is a [DNS CL](https://chromium-review.googlesource.com/c/chromium/src/+/6652887) that modifies the external file and locally passes the idlharness test w/o these diffs.
Related details
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 1 comment![]( "Open in Gerrit")
We know about one exposed regression here which seems too contrived to
cause any compat concerns: an `eval()` on a malformed
`element.scroll*()` call no longer throws synchronously; instead it now
returns a Promise synchronously which throws asynchronously.FYI vmpstr@.
Related details
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladimir Levin (Gerrit)
Vladimir Levin added 2 comments![]( "Open in Gerrit")
We know about one exposed regression here which seems too contrived to
cause any compat concerns: an `eval()` on a malformed
`element.scroll*()` call no longer throws synchronously; instead it now
returns a Promise synchronously which throws asynchronously.FYI vmpstr@.
File a CSSWG issue for this explaining that it's a consequence of the resolution and proposing that we accept this change
[FAIL] Element interface: operation scrollIntoView(optional (boolean or ScrollIntoViewOptions))Mustaq AhmedIf we resolve the promise, do these go away?
Vladimir LevinThe error remains the same with "always resolve". Even the first diff above ("unhandled rejection") remains.
Mustaq AhmedWhy is this error happening?
The idlharness test confirms that the exposed `element.scroll*` methods match the *external* spec-ed idl definitions in `external/wpt/interfaces/cssom-view.idl`. The external one would change only after my [PR](https://github.com/w3c/csswg-drafts/pull/12355) lands.
Here is a [DNS CL](https://chromium-review.googlesource.com/c/chromium/src/+/6652887) that modifies the external file and locally passes the idlharness test w/o these diffs.
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 1 comment![]( "Open in Gerrit")
We know about one exposed regression here which seems too contrived to
cause any compat concerns: an `eval()` on a malformed
`element.scroll*()` call no longer throws synchronously; instead it now
returns a Promise synchronously which throws asynchronously.Vladimir LevinFYI vmpstr@.
File a CSSWG issue for this explaining that it's a consequence of the resolution and proposing that we accept this change
I highlighted the case [here](https://github.com/w3c/csswg-drafts/issues/1562#issuecomment-2997993316), in the existing issue.
Related details
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladimir Levin (Gerrit)
Vladimir Levin voted Code-Review+1![]( "Open in Gerrit")
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
18 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/web_tests/external/wpt/css/cssom-view/idlharness-expected.txt
Insertions: 1, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Change information
Promisify scroll: element.idl changes behind a flag.
While the IDL change here cannot be gated directly by the RTE flag
`ProgrammaticScrollPromise` due to Blink IDL limitations, the return
values exposed to JS remains `undefined` through the use of
`EmptyPromise`.
We know about one exposed regression here which seems too contrived to
cause any compat concerns: a malformed `element.scroll*()` call no
longer throws synchronously; instead it now returns a Promise
synchronously which throws asynchronously.
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/dom/element.h
- M third_party/blink/renderer/core/dom/element.idl
- M third_party/blink/renderer/core/paint/cull_rect_updater_test.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/external/wpt/css/cssom-view/idlharness-expected.txt
Code-Review: +1 by Vladimir Levin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
Message from luci-bi...@appspot.gserviceaccount.com![]( "Open in Gerrit")
LUCI Bisection has identified this change as the cause of a test failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/test-analysis/b/5162228857700352
Sample build with failed test: https://ci.chromium.org/b/8711159289011790049
Affected test(s):
[ninja://:headless_shell_wpt/virtual/threaded-prefer-compositing/external/wpt/css/cssom-view/idlharness.html](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2F:headless_shell_wpt%2Fvirtual%2Fthreaded-prefer-compositing%2Fexternal%2Fwpt%2Fcss%2Fcssom-view%2Fidlharness.html?q=VHash%3A7a1dc22ad11924b1)
A revert for this change was not created because there are merged changes depending on it.
If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Ftest-analysis%2Fb%2F5162228857700352&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F6318459&type=BUG
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |