1825222 - Fix wpt failures in range-restore-oninput-onchange-event.html

Status: NEW

(Core :: DOM: Forms, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

wpt.fyi result: https://wpt.fyi/results/html/semantics/forms/the-input-element/range-restore-oninput-onchange-event.https.html

We fail at these cases:
Verifies that form restoration does not fire input or change events for <input type=range>.
Verifies that form restoration does not fire input or change events for <input type=text>.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

hey Vincent, please help this bug. Thank you.

Comment 2

[Vincent Hilla [:vhilla]]

Bug 1783245 synced a change from wpt that added a cache-control: no-store header causing this failure.

Comment 3

[Vincent Hilla [:vhilla]]

I did some comparison running the test with and without no-store.

The forms are restored via nsGenericHTMLFormControlElementWithState::RestoreFormControlState. This method uses nsLayoutHistoryState::GetState to retreive the state, then calls HTMLInputElement::RestoreState, which restores it's value from PresState::contentData.

With no-store, nsLayoutHistoryState::mScrollPositionOnly is true, thus GetState will clear PresState::contentData. The check for no-store happens in nsDocShell::ShouldDiscardLayoutState.

I'm unsure about the impact changes to ShouldDiscardLayoutState or GetState might have.

From a quick test, disabling the no-store check in ShouldDiscardLayoutState will also allow bfcache with no-store. Bug 261312 discussed about removing the no-store check, but opted for WONTFIX for compatibility reasons with Chrome. Though Chrome is now in the process of changing their behavior, see here.

Alternatively, I guess mScrollPositionOnly refers to #restore-persisted-state and from my understanding, everything in contentData (see PresState) falls under aspects that the UA previously had recorded in persisted user state, so it should be restored. Thus everything about mScrollPositionOnly could be removed. Though Bug 215405 introduced this flag explicitly to restore scroll position, but not form contents for no-store.

:farre, you are a peer of session restore, can you comment on this?

Comment 4

[Andreas Farre [:farre]]

Summary from video meeting: try to restore as much as possible regardless of no-store.

Comment 5

[Vincent Hilla [:vhilla]]

Removing everything about mScrollPositionOnly breaks the fix for Bug 1752250.

Comment 6

[Vincent Hilla [:vhilla]]

So to recap, this test aims to ensure that input and change events are not fired on form restoration. It therefore disables bfcache and performs a backward navigation to trigger form restoration.

WebKit updated this test to include no-store so that BFCache is disabled for them (see here). Blink and Gecko don't need this, as they already disable BFCache because the page has an opener.

The interaction between no-store and BFCache as well as form restoration is not clearly defined. Currently, no-store disables BFCache in Blink, Gecko and WebKit, though Blink is in the process of changing this (see here). For page reload, Blink does not restore forms while Gecko only does for non-no-store pages. For backward navigation in no-store documents, forms are restored in Blink and WebKit, but not in Gecko.

I believe, we want to change at least the last part, i.e. backward navigation in no-store documents should still restore forms. I might separate the updates to mScrollPositionOnly from the setters for mSaveLayoutState. Or, as not all setters for mSaveLayoutState also update mScrollPositionOnly, I could try to remove that update from the right setter without regressing Bug 1752250.

:peterv, you worked on Bug 1740517 that further prevented form restoration for no-store documents. Is it save to change this behavior and do you have an idea how I could approach this?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I'd be ok to follow blink's behavior both in reload and session history navigation. Especially in the reload case.
I believe there are some old bugs filed about that issue.

Comment 8

[Peter Van der Beken [:peterv]]

Discussed this a bit with :smaug, we think we can live with ignoring no-store for form restoration. So the new behaviour would be:

• reload -> no form restoration
• back/forward -> form restoration

We're wondering if the most elegant way is not to just clear the form data when we start a reload (keep the scroll info). We might be able to get rid of the mScrollPositionOnly then too.

Comment 9

[Peter Van der Beken [:peterv]]

I think that when SHIP is enabled, we can clear the layout history state of the entry that we're reloading in CanonicalBrowsingContext::NotifyOnHistoryReload. And get rid of nsDocShell::ShouldDiscardLayoutState.
Still need to verify where we need changes when SHIP is not enabled.