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@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