[PATCH 01/14] mshtml: Retry with VT_NULL when removing an attribute.

[Gabriel Ivăncescu]

On 16/11/2021 18:47, Jacek Caban wrote:
> 
> On 16/11/2021 16:48, Gabriel Ivăncescu wrote:
>> Hi Jacek,
>>
>> On 16/11/2021 16:57, Jacek Caban wrote:
>>> Hi Gabriel,
>>>
>>> On 11/16/21 3:29 PM, Gabriel Ivăncescu wrote:
>>>> diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c
>>>> index 96a776d..bddb0e3 100644
>>>> --- a/dlls/mshtml/dispex.c
>>>> +++ b/dlls/mshtml/dispex.c
>>>> @@ -1466,6 +1466,11 @@ HRESULT remove_attribute(DispatchEx *This, 
>>>> DISPID id, VARIANT_BOOL *success)
>>>>           V_VT(&var) = VT_EMPTY;
>>>>           hres = builtin_propput(This, func, &dp, NULL);
>>>> +        if(hres == E_NOTIMPL) {
>>>> +            /* Setting event handlers to VT_EMPTY fails in quirks 
>>>> mode */
>>>> +            V_VT(&var) = VT_NULL;
>>>> +            hres = builtin_propput(This, func, &dp, NULL);
>>>> +        }
>>>>           if(FAILED(hres))
>>>>               return hres;
>>>> diff --git a/dlls/mshtml/tests/documentmode.js 
>>>> b/dlls/mshtml/tests/documentmode.js
>>>> index d566223..c197f9b 100644
>>>> --- a/dlls/mshtml/tests/documentmode.js
>>>> +++ b/dlls/mshtml/tests/documentmode.js
>>>> @@ -1087,6 +1087,46 @@ sync_test("elem_attr", function() {
>>>>       ok(r === "cls2", "class attr = " + r);
>>>>       r = elem.getAttribute("className");
>>>>       ok(r === "cls3", "className attr = " + r);
>>>> +
>>>> +    var func = function() { };
>>>> +    elem.onclick = func;
>>>> +    ok(elem.onclick === func, "onclick = " + elem.onclick);
>>>> +    r = elem.getAttribute("onclick");
>>>> +    ok(r === (v < 8 ? func : null), "onclick attr = " + r);
>>>> +    r = elem.removeAttribute("onclick");
>>>> +    todo_wine_if(v < 9).
>>>> +    ok(r === (v < 9 ? false : undefined), "removeAttribute returned 
>>>> " + r);
>>>> +    todo_wine_if(v < 9).
>>>> +    ok(elem.onclick === (v != 8 ? func : null), "removed onclick = 
>>>> " + elem.onclick);
>>>
>>>
>>> This test shows that removeAttribute() should not work in this case 
>>> (although it should not throw), so making it work does not seem right.
>>>
>>>
>>> Thanks,
>>>
>>> Jacek
>>>
>>
>> Well, in subsequent tests, it does work. The reason it doesn't work 
>> here is because the attribute is set to a non-string value (there's 
>> tests + fixes later in the series for that). Should I move the tests 
>> with todo_wine then?
> 
> 
> I would hope for something cleaner instead. So far this patch and patch 
> 5 is backed only on event handler tests and event handlers are likely to 
> behave differently than other properties. I can't be sure without 
> trying, but for those tests it seems to me that you could replace 
> throwing by returning false from remove_attribute and try to use 
> dispex_get_dprop_ref to clear the value (which would cover string values 
> of event handler).
> 
> 
> Thanks,
> 
> Jacek
> 
> 

Interesting. I considered that at first, but I actually thought that 
using event-handler implementation details would be "less clean". :-)

Your suggestion should be good enough, though. I don't think we'll ever 
know exactly, because the event handlers are the only read-write builtin 
props in quirks modes that use VT_VARIANT instead of something more 
specific, so it doesn't matter in the end.

Also, the toString is (surprisingly) correct, but I'll resend with extra 
tests of course, and move them around earlier where possible.