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

[Jacek Caban]

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