-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Comments
Note that Gecko has a bug here as well with computing |
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 ? |
Yeah, I think that's basically what the current spec proposes that we do.
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 . |
After looking at the chromium implementation of I can see a couple of ways the behavior could work:
(apologies if the spec is already abundantly clear on this) |
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. |
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 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 . |
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 |
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}
I landed the fix in chromium! Thanks again for the jsdom reference |
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) |
I think https://bugs.webkit.org/show_bug.cgi?id=196532 is the relevant WebKit bug. |
All browsers landed their fixes. 🥳 |
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
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.
The text was updated successfully, but these errors were encountered: