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.
Your attached patch has removeAttribute return true in both native and wine. It also doesn't even use this code path, taking it out results in the same test failures in wine (with the extra one being the one fixed by the patch of course).
So what this patch is about: when an event handler fails setting to VT_EMPTY, and then it returns false (tested previously), it still has to set it to NULL. There is one other type of attribute which will use this code path: read-only props. However, for read-only props, the set-to-NULL will fail and do nothing anyway, so it doesn't require special casing.
Instead, what seems to be the problem here is that removeAttribute, when it returns true (i.e. not this patch's code path) should reset the underlying props to their default values. This seems unrelated and a completely separate change to me.
It would be hard to fix as well, since it requires doing it in every builtin prop, possibly via some hook (I don't know if we can always detect removeAttribute from input parameter, or whether it's "different" in behavior than setting it to VT_EMPTY or not manually).
Doesn't seem worth to do right now, I was focusing on event handlers since that's what were used in some js libraries.
I think the patch is good enough for what it does: set event handlers to NULL when removeAttribute returns false. Other behavior is, imo, deserving separate changes, and not about this code path at all.
Thanks, Gabriel