LibWeb: Correct initiator origin logic for new top level traversables#8908
LibWeb: Correct initiator origin logic for new top level traversables#8908jonbgamble wants to merge 1 commit intoLadybirdBrowser:masterfrom
Conversation
|
|
||
| // initiator origin: null if opener is null; otherwise, document's origin | ||
| document_state->set_initiator_origin(opener ? Optional<URL::Origin> {} : document->origin()); | ||
| document_state->set_initiator_origin(opener ? document->origin() : Optional<URL::Origin> {}); |
There was a problem hiding this comment.
oof nice spot, any possbility of adding a test for this?
jonbgamble
force-pushed
the
pr-document-state-origin
branch
2 times, most recently
from
9b2939d to
ead7cef
Compare
|
Just noting so it doesn't get lost - the test added here failed in one of the runs: --- /home/runner/_work/ladybird/ladybird/Tests/LibWeb/Text/expected/./HTML/window-open-about-blank-inherits-opener-origin.txt
+++ /home/runner/_work/ladybird/ladybird/Tests/LibWeb/Text/expected/./HTML/window-open-about-blank-inherits-opener-origin.txt
@@ -1,3 +1,3 @@
initial popup.location.href: about:blank
initial popup.document.body.textContent: Hello Shannon!
-reloaded popup.document.body.textContent:
+reloaded popup.document.body.textContent: Hello Shannon! |
|
Nice one. I don't suppose I could blame this on Shannon? |
jonbgamble
force-pushed
the
pr-document-state-origin
branch
from
ead7cef to
cf9f0ed
Compare
| } | ||
|
|
||
| await timeout(10); | ||
| } |
There was a problem hiding this comment.
I believe WPT's standard way of dealing with these types of updates should work immediately; i.e. two nested requestAnimationFrame(..)s and a setTimeout(.., 0) inside of that.
There was a problem hiding this comment.
For my understanding (I was wondering how we could do this) what is the magic which makes that timing okay? (specifically 2 animation frames)
There was a problem hiding this comment.
As I understand, the first rAF triggers within the current frame (nothing updated yet), the second one actually enforces executing on the next rendering opportunity (layout/style updated) but might not yet have triggered the painting logic to the point where compositing is done, so the inner setTimeout() makes sure we schedule a task to let painting do its thing and taking a screenshot will reliably work.
For different test cases different combinations of these three might work equally well, but rAF+rAF+setTimeout has been easy to remember as something that "just works" for me. For screenshot tests I can confirm that leaving out any of the three has caused tests to be flaky in the past.
There was a problem hiding this comment.
I could try it, but we're waiting on the popup reload to replace the Document, which likely takes longer than waiting for renders to settle.
4 participants
No description provided.