Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output element value/defaultValue/reset() is not interoperable #6516

Closed
domenic opened this issue Mar 20, 2021 · 11 comments
Closed

Output element value/defaultValue/reset() is not interoperable #6516

domenic opened this issue Mar 20, 2021 · 11 comments
Labels
interop Implementations are not interoperable with each other topic: forms

Comments

@domenic
Copy link
Member

domenic commented Mar 20, 2021

Blink and WebKit do not align with the spec's simple "computed" model for determining value and defaultValue. Instead they seem to use child observers (not descendant observers) to sync the default value with the descendant text content.

This causes them to fail the relevant test, although that test is broken anyway further down in the assert chain. (The latter is being fixed in web-platform-tests/wpt#28161.)

A stripped down demonstration is https://jsbin.com/degojafare/1/edit?html,output

IMO this is best classified as a Blink/WebKit bug as it's super-weird that they do not take descendant modifications into account, only child mutations. Also, the spec behavior (which Gecko matches) is much nicer and simpler as it doesn't require any observers at all.

Any thoughts @rniwa @josepharhar? I am pretty sure this is an extreme edge case so aligning Blink and WebKit to Gecko and the spec shouldn't cause compat problems.

Also, Gecko does not match the spec/Blink/WebKit's reset behavior, as seen from the wpt.fyi dashboard, so there's plenty of non-interop to go around here.

@domenic domenic added topic: forms interop Implementations are not interoperable with each other labels Mar 20, 2021
@annevk
Copy link
Member

annevk commented Mar 23, 2021

Note that Gecko has a bug here as well with computing defaultValue: https://bugzilla.mozilla.org/show_bug.cgi?id=1537689. http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2835 in particular. (Safari does as well, Chrome does not.)

@josepharhar
Copy link
Contributor

Hmm... if we aren't listening to child mutations enough which assign textContent to defaultValue, why not just calculate and return textContent every time defaultValue is called?

Is there already a WPT for http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2835 ?

@domenic
Copy link
Member Author

domenic commented Mar 24, 2021

Hmm... if we aren't listening to child mutations enough which assign textContent to defaultValue, why not just calculate and return textContent every time defaultValue is called?

Yeah, I think that's basically what the current spec proposes that we do.

Is there already a WPT for http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2835 ?

It's hard to tell because the current test (this one) tests a lot of things at once. So IMO it would be nice to add a dedicated extra test derived from http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2835 .

@domenic domenic added the agenda+ To be discussed at a triage meeting label Mar 31, 2021
@josepharhar
Copy link
Contributor

After looking at the chromium implementation of <output> more, it looks like there is a bunch of logic around the "default value mode" - until the element.value property gets assigned to, assigning to element.defaultValue changes the textContent. After element.value gets assigned to, changes to the textContent which could affect element.defaultValue are ignored...

I can see a couple of ways the behavior could work:

  1. make defaultValue unconditionally compute and return textContent, remove HTMLOutputElement's ChildrenChanged listener which assigns to defaultValue, and delete HTMLOutputElement's saved defaultValue variable. Possibly also delete the "default value mode" concept/state entirely.
  2. keep the current ChildrenChanged listener which assigns to defaultValue, and when element.defaultValue is called, recompute textContent and assign it to HTMLOutputElement's saved defaultValue variable, but only when we are in "default value mode". This means that after element.value gets assigned to and the mode changes, you will get whatever defaultValue was the last time element.defaultValue was called...

(apologies if the spec is already abundantly clear on this)

@josepharhar
Copy link
Contributor

josepharhar commented Apr 23, 2021

A third option, which is probably the best, would be to figure out how to use something other than the ChildrenChanged listener to listen to child mutations - I'm not sure how I'd implement it yet though... from what code I've worked with in the DOM so far, this basically isn't possible because there is no internal equivalent to mutation events.

@domenic
Copy link
Member Author

domenic commented Apr 23, 2021

I suspect the "default value mode" concept is important for compatibility. But otherwise your (1) is on the right track.

Here is an implementation of the current spec which uses the "default value mode" concept plus a "default value" string: https://github.com/jsdom/jsdom/blob/5e39a4c60377c95ac09ea938ecbf5f1f91cb0360/lib/jsdom/living/nodes/HTMLOutputElement-impl.js

The important pieces are the value getter/setter, the defaultValue getter/setter, and the form reset handler. You could refactor Chromium to look like that without much trouble I think, and it would only change the strange cases discussed in the OP where descendants are updated and Chromium would now take them into account (by recomputing defaultValue based on textContent) instead of ignoring them (because today it only listens for child changes, not descendant changes).

Here is an implementation of the current spec which adheres very closely to the current spec, by using a nullable "default value override" instead of a boolean "default value mode" + "default value" string: https://github.com/jsdom/jsdom/blob/04f6c13f4a4d387c7fc979b8f62c6f68d8a0c639/lib/jsdom/living/nodes/HTMLOutputElement-impl.js

It is equivalent. You can see the diff in jsdom/jsdom@761d8cc .

@domenic domenic removed the agenda+ To be discussed at a triage meeting label May 6, 2021
@josepharhar
Copy link
Contributor

Thanks for the jsdom reference! I made a patch for chromium which appears to work here: https://chromium-review.googlesource.com/c/chromium/src/+/2878397

blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue May 12, 2021
The ChildrenChanged listener which updates defaultValue doesn't account
for mutations deeper than the first layer of children. This patch
accounts for it by computing textContent more often instead of relying
on ChildrenChanged.

This was brought up in this whatwg/html discussion [1]. Based on
domenic's suggestion [2], I heavily based this on the jsdom
implementation [3].

[1] whatwg/html#6516
[2] whatwg/html#6516 (comment)
[3] https://github.com/jsdom/jsdom/blob/04f6c13f4a4d387c7fc979b8f62c6f68d8a0c639/lib/jsdom/living/nodes/HTMLOutputElement-impl.js

Change-Id: I2f491f3c2759f1d3ca4fce1025f5677e34479d6f
Fixed: 1206416
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2878397
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#881932}
@josepharhar
Copy link
Contributor

I landed the fix in chromium! Thanks again for the jsdom reference

@rniwa
Copy link

rniwa commented May 12, 2021

FWIW, this does seem like a bug. Did you file a WebKit bug somewhere? (not asking to file one if you already haven't done so)

@domenic
Copy link
Member Author

domenic commented May 12, 2021

I think https://bugs.webkit.org/show_bug.cgi?id=196532 is the relevant WebKit bug.

@annevk
Copy link
Member

annevk commented May 18, 2021

All browsers landed their fixes. 🥳

@annevk annevk closed this as completed May 18, 2021
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
The ChildrenChanged listener which updates defaultValue doesn't account
for mutations deeper than the first layer of children. This patch
accounts for it by computing textContent more often instead of relying
on ChildrenChanged.

This was brought up in this whatwg/html discussion [1]. Based on
domenic's suggestion [2], I heavily based this on the jsdom
implementation [3].

[1] whatwg/html#6516
[2] whatwg/html#6516 (comment)
[3] https://github.com/jsdom/jsdom/blob/04f6c13f4a4d387c7fc979b8f62c6f68d8a0c639/lib/jsdom/living/nodes/HTMLOutputElement-impl.js

Change-Id: I2f491f3c2759f1d3ca4fce1025f5677e34479d6f
Fixed: 1206416
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2878397
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#881932}
NOKEYCHECK=True
GitOrigin-RevId: 922348f2d63373118a570373df1fc5c949c5a7d2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: forms
Development

No branches or pull requests

4 participants