Changeset 290841 in webkit


Ignore:
Timestamp:
Mar 4, 2022 12:35:01 PM (5 months ago)
Author:
Chris Dumez
Message:

Load event never firing after form is submitted
​https://bugs.webkit.org/show_bug.cgi?id=235407
<rdar://problem/87831049>

Reviewed by Geoffrey Garen.

LayoutTests/imported/w3c:

Rebaseline WPT tests that are no longer timing out.

  • web-platform-tests/html/semantics/forms/form-submission-target/rel-base-target-expected.txt:
  • web-platform-tests/html/semantics/forms/form-submission-target/rel-button-target-expected.txt:
  • web-platform-tests/html/semantics/forms/form-submission-target/rel-form-target-expected.txt:
  • web-platform-tests/html/semantics/forms/form-submission-target/rel-input-target-expected.txt:

Source/WebCore:

In Document::implicitClose(), we early return (and thus don't fire the load
event) if there is a location change pending. To determine if there is a
location change pending, we rely on NavigationScheduler::locationChangePending()
which checks if there is a schedule navigation or not. This usually works fine.

However, when a form gets submitted with a target that is "_blank",
FrameLoader::submitForm() is not able to find the target frame (since we'll need
to create one) and it ends up using the current frame's scheduler. The idea is
that once the navigation actually triggers, FrameLoader::loadFrameRequest() will
check the target and create the new Frame.

The issue is that as a result of this, NavigationScheduler::locationChangePending()
returns true for the submitter's frame while such form submission is scheduled,
even though the navigation will actually happen in another (new) frame. To address
the issue, I updated NavigationScheduler::locationChangePending() to check that
the pending navigation is actually for the current frame.

Test: http/tests/loading/form-submission-no-load-event.html

  • loader/NavigationScheduler.cpp:

(WebCore::ScheduledNavigation::targetIsCurrentFrame const):
(WebCore::NavigationScheduler::locationChangePending):
(WebCore::ScheduledFormSubmission::ScheduledFormSubmission): Deleted.

LayoutTests:

Add layout test coverage (Based on reduction from Sam Sneddon).

  • http/tests/loading/form-submission-no-load-event-expected.txt: Added.
  • http/tests/loading/form-submission-no-load-event.html: Added.
Location:
trunk
Files:
2 added
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r290839 r290841  
     12022-03-04  Chris Dumez  <cdumez@apple.com>
     2
     3        Load event never firing after form is submitted
     4        https://bugs.webkit.org/show_bug.cgi?id=235407
     5        <rdar://problem/87831049>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        Add layout test coverage (Based on reduction from Sam Sneddon).
     10
     11        * http/tests/loading/form-submission-no-load-event-expected.txt: Added.
     12        * http/tests/loading/form-submission-no-load-event.html: Added.
     13
    1142022-03-04  Said Abou-Hallawa  <said@apple.com>
    215

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r290831 r290841  
     12022-03-04  Chris Dumez  <cdumez@apple.com>
     2
     3        Load event never firing after form is submitted
     4        https://bugs.webkit.org/show_bug.cgi?id=235407
     5        <rdar://problem/87831049>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        Rebaseline WPT tests that are no longer timing out.
     10
     11        * web-platform-tests/html/semantics/forms/form-submission-target/rel-base-target-expected.txt:
     12        * web-platform-tests/html/semantics/forms/form-submission-target/rel-button-target-expected.txt:
     13        * web-platform-tests/html/semantics/forms/form-submission-target/rel-form-target-expected.txt:
     14        * web-platform-tests/html/semantics/forms/form-submission-target/rel-input-target-expected.txt:
     15
    1162022-03-04  Antoine Quint  <graouts@webkit.org>
    217

  • trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-base-target-expected.txt

    r284749 r290841  
    1 
    2 Harness Error (TIMEOUT), message = null
    31
    42PASS <form rel=""> with <base target>

  • trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-button-target-expected.txt

    r284749 r290841  
    1 
    2 Harness Error (TIMEOUT), message = null
    31
    42PASS <form rel=""> with <button formtarget>

  • trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-form-target-expected.txt

    r284749 r290841  
    1 
    2 Harness Error (TIMEOUT), message = null
    31
    42PASS <form rel=""> with <form target>

  • trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-target/rel-input-target-expected.txt

    r284749 r290841  
    1 
    2 Harness Error (TIMEOUT), message = null
    31
    42PASS <form rel=""> with <input formtarget>

  • trunk/Source/WebCore/ChangeLog

    r290839 r290841  
     12022-03-04  Chris Dumez  <cdumez@apple.com>
     2
     3        Load event never firing after form is submitted
     4        https://bugs.webkit.org/show_bug.cgi?id=235407
     5        <rdar://problem/87831049>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        In Document::implicitClose(), we early return (and thus don't fire the load
     10        event) if there is a location change pending. To determine if there is a
     11        location change pending, we rely on NavigationScheduler::locationChangePending()
     12        which checks if there is a schedule navigation or not. This usually works fine.
     13
     14        However, when a form gets submitted with a target that is "_blank",
     15        FrameLoader::submitForm() is not able to find the target frame (since we'll need
     16        to create one) and it ends up using the current frame's scheduler. The idea is
     17        that once the navigation actually triggers, FrameLoader::loadFrameRequest() will
     18        check the target and create the new Frame.
     19
     20        The issue is that as a result of this, NavigationScheduler::locationChangePending()
     21        returns true for the submitter's frame while such form submission is scheduled,
     22        even though the navigation will actually happen in another (new) frame. To address
     23        the issue, I updated NavigationScheduler::locationChangePending() to check that
     24        the pending navigation is actually for the current frame.
     25
     26        Test: http/tests/loading/form-submission-no-load-event.html
     27
     28        * loader/NavigationScheduler.cpp:
     29        (WebCore::ScheduledNavigation::targetIsCurrentFrame const):
     30        (WebCore::NavigationScheduler::locationChangePending):
     31        (WebCore::ScheduledFormSubmission::ScheduledFormSubmission): Deleted.
     32
    1332022-03-04  Said Abou-Hallawa  <said@apple.com>
    234

  • trunk/Source/WebCore/loader/NavigationScheduler.cpp

    r285214 r290841  
    9393    virtual void didStartTimer(Frame&, Timer&) { }
    9494    virtual void didStopTimer(Frame&, NewLoadInProgress) { }
     95    virtual bool targetIsCurrentFrame() const { return true; }
    9596
    9697    double delay() const { return m_delay; }
    … …  
    291292};
    292293
    293 class ScheduledFormSubmission : public ScheduledNavigation {
     294class ScheduledFormSubmission final : public ScheduledNavigation {
    294295public:
    295296    ScheduledFormSubmission(Ref<FormSubmission>&& submission, LockBackForwardList lockBackForwardList, bool duringLoad)
    … …  
    299300    }
    300301
    301     void fire(Frame& frame) override
     302    void fire(Frame& frame) final
    302303    {
    303304        if (m_submission->wasCancelled())
    … …  
    324325    }
    325326
    326     void didStartTimer(Frame& frame, Timer& timer) override
     327    void didStartTimer(Frame& frame, Timer& timer) final
    327328    {
    328329        if (m_haveToldClient)
    … …  
    334335    }
    335336
    336     void didStopTimer(Frame& frame, NewLoadInProgress newLoadInProgress) override
     337    void didStopTimer(Frame& frame, NewLoadInProgress newLoadInProgress) final
    337338    {
    338339        if (!m_haveToldClient)
    … …  
    348349    }
    349350
     351    bool targetIsCurrentFrame() const final
     352    {
     353        // For form submissions, we normally resolve the target frame before scheduling the submission on the
     354        // NavigationScheduler. However, if the target is _blank, we schedule the submission on the submitter's
     355        // frame and only create the new frame when actually starting the navigation.
     356        return !isBlankTargetFrameName(m_submission->target());
     357    }
     358
    350359private:
    351360    Ref<FormSubmission> m_submission;
    … …  
    396405bool NavigationScheduler::locationChangePending()
    397406{
    398     return m_redirect && m_redirect->isLocationChange();
     407    return m_redirect && m_redirect->isLocationChange() && m_redirect->targetIsCurrentFrame();
    399408}
    400409
Note: See TracChangeset for help on using the changeset viewer.