On 02/12/2021 15:51, Jacek Caban wrote:
On 12/2/21 2:20 PM, Gabriel Ivăncescu wrote:
On 01/12/2021 21:24, Jacek Caban wrote:
Hi Gabriel,
On 12/1/21 2:41 PM, Gabriel Ivăncescu wrote:
- if(dispex_compat_mode(This) >= COMPAT_MODE_IE8) {
+ V_VT(&var) = VT_NULL; + builtin_propput(This, func, &dp, NULL); + }
That doesn't seem universally right, see the attached test for an example.
Thanks,
Jacek
Are you sure this is related to this patch? I did some digging but it seems to be a completely different issue to me. This patch is about removeAttribute returning false and setting to NULL, which happens with event handlers.
Yes, the existing code is already wrong, we probably shouldn't blindly try to set value to VT_EMPTY in the first place. Trying to set the property to another value (of type that doesn't make sense unless you assume that those are event handlers) based on a previous error that we know nothing about (it could be OOM?) is making it even worse. Is there another reason for this patch than a todo_wine in tests?
Thanks,
Jacek
I think the VT_EMPTY is correct for pre IE8 modes, since I didn't get any failures there, just not for IE8.
Even if we assume that we do implement it correctly for IE8 to reset it to default values at some point, it would *still* not work without this patch for this particular case, since that would handle the case where it returns true, not false. Isn't this patch still needed, then? I can't see how else it would work.
As for the reason, well, prototype.js sets "onclick" to an array and later removeAttribute on it. Not sure to what extent it's useful though. But it was a pretty simple fix that I honestly see no downsides to—even in the future should we implement the rest properly.