script: implement missing steps for form submission#43700
Conversation
kkoyung
left a comment
There was a problem hiding this comment.
A few nits.
We usually prefix the PR title with crate you are working on. In this case, replace "feat:" with "script:" in the PR title.
I see, I'll make an edit |
|
🔨 Triggering try run (#23632948414) for Linux (WPT) |
|
Test results for linux-wpt from try job (#23632948414): Flaky unexpected result (29)
Stable unexpected results that are known to be intermittent (13)
Stable unexpected results (9)
|
|
|
Also update the test expectation according to the report. |
Okay |
| let history_handling = if doc == target_document && !doc.completely_loaded() { | ||
| let history_handling = if std::ptr::eq(&*doc as *const _, &*target_document as *const _) && | ||
| !doc.completely_loaded() | ||
| { |
There was a problem hiding this comment.
Is there any particular reason to switch from using == to comparing the pointer instead, at the last commit?
There was a problem hiding this comment.
Okay so while updating test expectations, I noticed jsurl-form-submit.tentative.html was failing and history_handling was becoming Replace when it should be Auto. Suspecting == on DomRoot wasn't doing pointer equality, so I tried std::ptr::eq instead to see what happens.
There was a problem hiding this comment.
They do that already:
servo/components/script_bindings/root.rs
Line 173 in 54bc366
There was a problem hiding this comment.
Oh okay, thanks for the clarification! I'll revert back to ==. I'm still not sure why jsurl-form-submit.tentative.html was failing though, is that a pre-existing issue or something my change introduced?
There was a problem hiding this comment.
I'm confused—the changes in this PR make it look like the test is passing since we're removing the ini file. Do you see different results locally?
f195a41 to
7b01eb5
Compare
|
@TG199 Could you look into the tests that are now timing out like /html/browsers/browsing-the-web/navigating-across-documents/replace-before-load/form-submit.html? You can use https://book.servo.org/contributing/guides/diagnosing-errors/stable-wpt-errors.html?highlight=Timeo#diagnosing-a-timeout-result for inspiration. |
|
Hi @jdm I've confirmed this is a regression from my change. On main the test passes but on my branch it times out. The test submits a form inside an iframe while it's still loading. My step 25 sets |
|
What's the step in the test that times out? I'm assuming it's waiting for some event. |
|
The timeout is in |
This is very helpful! That looks like a preexisting bug in Servo that I've been able to reproduce, so I'm going to file a new issue for that. |
Okay |
|
Since the timeout is related to another bug, we can fix it separately. Apart from that, the PR looks good to me. @TG199 Could you update the test expectation so that we can run the tests once more on our CI? The These are the related tests: |
Thank you for pointing out the related test directories! I've updated the expectations and pushed. |
e637e22 to
698e299
Compare
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
… comparison Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
698e299 to
fb693d8
Compare
|
@jdm is this good as is? |
Signed-off-by: Kelechi Ebiri <ebiritg@gmail.com>
|
🔨 Triggering try run (#24254482947) for Linux (WPT) |
|
Test results for linux-wpt from try job (#24254482947): Flaky unexpected result (26)
Stable unexpected results that are known to be intermittent (20)
|
|
✨ Try run (#24254482947) succeeded. |
Implement missing history handling steps for form submission
Test: ./mach test-wpt tests/wpt/tests/html/semantics/forms/form-submission-0
Fixes: #43670