[PATCH v10 2/5] mshtml: Fix setting and retrieving attributes in IE8 mode.

[Gabriel Ivăncescu]

On 29/11/2021 16:15, Jacek Caban wrote:
> On 11/29/21 2:14 PM, Gabriel Ivăncescu wrote:
>> For non-builtin props, getAttribute retrieves the stringified value of 
>> the
>> prop. For builtins, however, getAttribute returns null unless they were
>> set to a string. setAttribute also stringifies the value if it's a 
>> builtin.
>>
>> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
>> ---
>>
>> I'm testing for/htmlFor since prototype.js also uses it for translations,
>> but it doesn't seem to be affected here.
>>
>>   dlls/mshtml/htmlelem.c            | 193 ++++++++++++++++++++----------
>>   dlls/mshtml/tests/documentmode.js |  76 +++++++++---
>>   2 files changed, 188 insertions(+), 81 deletions(-)
>>
>> diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c
>> index 3f7c60b..bd9dc29 100644
>> --- a/dlls/mshtml/htmlelem.c
>> +++ b/dlls/mshtml/htmlelem.c
>> @@ -1067,6 +1067,15 @@ static HRESULT WINAPI 
>> HTMLElement_Invoke(IHTMLElement *iface, DISPID dispIdMembe
>>               wFlags, pDispParams, pVarResult, pExcepInfo, puArgErr);
>>   }
>> +static inline WCHAR *translate_attr_name(WCHAR *attr_name, 
>> compat_mode_t compat_mode)
>> +{
>> +    WCHAR *ret = attr_name;
>> +
>> +    if(compat_mode >= COMPAT_MODE_IE8 && !wcsicmp(attr_name, L"class"))
>> +        ret = (WCHAR*)L"className";
>> +    return ret;
>> +}
>> +
>>   static HRESULT set_elem_attr_value_by_dispid(HTMLElement *elem, 
>> DISPID dispid, VARIANT *v)
>>   {
>>       DISPID propput_dispid = DISPID_PROPERTYPUT;
>> @@ -1086,34 +1095,62 @@ static HRESULT WINAPI 
>> HTMLElement_setAttribute(IHTMLElement *iface, BSTR strAttr
>>                                                  VARIANT 
>> AttributeValue, LONG lFlags)
>>   {
>>       HTMLElement *This = impl_from_IHTMLElement(iface);
>> +    compat_mode_t compat_mode = 
>> dispex_compat_mode(&This->node.event_target.dispex);
>> +    nsAString name_str, value_str;
>> +    VARIANT val = AttributeValue;
>> +    BOOL needs_free = FALSE;
>> +    nsresult nsres;
>>       DISPID dispid;
>>       HRESULT hres;
>>       TRACE("(%p)->(%s %s %08x)\n", This, 
>> debugstr_w(strAttributeName), debugstr_variant(&AttributeValue), lFlags);
>> -    if(This->dom_element && 
>> dispex_compat_mode(&This->node.event_target.dispex) >= COMPAT_MODE_IE8) {
>> -        nsAString name_str, value_str;
>> -        nsresult nsres;
>> -
>> -        hres = variant_to_nsstr(&AttributeValue, FALSE, &value_str);
>> +    if(compat_mode < COMPAT_MODE_IE9 || !This->dom_element) {
> 
> 
> Why do you change IE9+ code path? Would changing the condition from IE8 
> to IE9 be enough for this code path?
> 

The code path is not changed, it's just restructured so that the 
GetDispID is at the top, because for IE8 case it has to "fallback" to 
IE9+ under certain conditions (for class and style) which are checked 
after GetDispID. Otherwise I'd have to use a goto which would be less 
than ideal, or duplicate the gecko get/setAttribute code.

Note that this special casing done for class is not a hack—javascript 
libraries that deal with browser quirks (prototype.js, jsquery, etc) 
have translation tables for this, so it's necessary.

Basically for IE8 it's like this:

1) Attributes have associated props, like in pre IE8 modes.

2) "class" changes className prop, while "className" doesn't change any 
prop but sets a separated attribute (same as IE9+, since "className" is 
a builtin prop which sets the "class" in the builtin). i.e. it needs 
special casing.

3) For attributes corresponding to builtin props, the value is 
stringified, but not for dynamic props. This is done after GetDispID.

4) When retrieving an attribute, if the attribute is a builtin and if 
the value is not a string, it returns null. For other attributes, the 
value is stringified.

Note that despite (3), it *is* possible to set a builtin attribute to a 
non-string: by setting the prop instead of using setAttribute. That's 
because attributes have corresponding props.

This also makes it possible to change the underlying prop's referenced 
value (such as an array element's value) to prove that getAttribute is 
what stringifies it, and it isn't just stringified when set, because it 
will return the new value, stringified.

This is in the tests already, of course, so it needs to pass.


Do you have some better idea how to structure the code then? Otherwise I 
think I'll just restructure it first on a no-op patch like this, and 
then add the fixed behavior.

> 
>>   static HRESULT WINAPI HTMLElement_put_className(IHTMLElement *iface, 
>> BSTR v)
>> @@ -6463,6 +6523,9 @@ static HRESULT 
>> HTMLElement_populate_props(DispatchEx *dispex)
>>       nsresult nsres;
>>       HRESULT hres;
>> +    if(dispex_compat_mode(dispex) >= COMPAT_MODE_IE9)
>> +        return S_OK;
> 
> 
> While the rest of the patch could be split as well, this part (while 
> probably correct) definitely doesn't belong to this patch.
> 
> 

I see, I remember having some troubles trying to split it by introducing 
new failures on existing tests, but I'll have a look again and see what 
can be done.

Thanks,
Gabriel