[PATCH v12 1/7] mshtml: Restructure element setAttribute.

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---

Will be needed for IE8 to fallback to the gecko attribute nodes under
certain conditions.

 dlls/mshtml/htmlelem.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c
index 3f7c60b..16357de 100644
--- a/dlls/mshtml/htmlelem.c
+++ b/dlls/mshtml/htmlelem.c
@@ -1086,34 +1086,36 @@ 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;
+    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_IE8 || !This->dom_element) {
+        hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, strAttributeName,
+                (lFlags&ATTRFLAG_CASESENSITIVE ? fdexNameCaseSensitive : fdexNameCaseInsensitive) | fdexNameEnsure, &dispid);
         if(FAILED(hres))
             return hres;
 
-        nsAString_InitDepend(&name_str, strAttributeName);
-        nsres = nsIDOMElement_SetAttribute(This->dom_element, &name_str, &value_str);
-        nsAString_Finish(&name_str);
-        nsAString_Finish(&value_str);
-        if(NS_FAILED(nsres))
-            WARN("SetAttribute failed: %08x\n", nsres);
-        return map_nsresult(nsres);
+        return set_elem_attr_value_by_dispid(This, dispid, &AttributeValue);
     }
 
-    hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, strAttributeName,
-            (lFlags&ATTRFLAG_CASESENSITIVE ? fdexNameCaseSensitive : fdexNameCaseInsensitive) | fdexNameEnsure, &dispid);
+    hres = variant_to_nsstr(&AttributeValue, FALSE, &value_str);
     if(FAILED(hres))
         return hres;
 
-    return set_elem_attr_value_by_dispid(This, dispid, &AttributeValue);
+    nsAString_InitDepend(&name_str, strAttributeName);
+    nsres = nsIDOMElement_SetAttribute(This->dom_element, &name_str, &value_str);
+    nsAString_Finish(&name_str);
+    nsAString_Finish(&value_str);
+    if(NS_FAILED(nsres))
+        WARN("SetAttribute failed: %08x\n", nsres);
+    hres = map_nsresult(nsres);
+
+    return hres;
 }
 
 HRESULT get_elem_attr_value_by_dispid(HTMLElement *elem, DISPID dispid, VARIANT *ret)
-- 
2.31.1

[PATCH v12 2/7] mshtml: Restructure element getAttribute.

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/mshtml/htmlelem.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c
index 16357de..b6c9db0 100644
--- a/dlls/mshtml/htmlelem.c
+++ b/dlls/mshtml/htmlelem.c
@@ -1158,6 +1158,9 @@ static HRESULT WINAPI HTMLElement_getAttribute(IHTMLElement *iface, BSTR strAttr
                                                LONG lFlags, VARIANT *AttributeValue)
 {
     HTMLElement *This = impl_from_IHTMLElement(iface);
+    compat_mode_t compat_mode = dispex_compat_mode(&This->node.event_target.dispex);
+    nsAString name_str, value_str;
+    nsresult nsres;
     DISPID dispid;
     HRESULT hres;
 
@@ -1166,33 +1169,28 @@ static HRESULT WINAPI HTMLElement_getAttribute(IHTMLElement *iface, BSTR strAttr
     if(lFlags & ~(ATTRFLAG_CASESENSITIVE|ATTRFLAG_ASSTRING))
         FIXME("Unsupported flags %x\n", lFlags);
 
-    if(This->dom_element && dispex_compat_mode(&This->node.event_target.dispex) >= COMPAT_MODE_IE8) {
-        nsAString name_str, value_str;
-        nsresult nsres;
-
-        nsAString_InitDepend(&name_str, strAttributeName);
-        nsAString_InitDepend(&value_str, NULL);
-        nsres = nsIDOMElement_GetAttribute(This->dom_element, &name_str, &value_str);
-        nsAString_Finish(&name_str);
-        return return_nsstr_variant(nsres, &value_str, 0, AttributeValue);
-    }
+    if(compat_mode < COMPAT_MODE_IE8 || !This->dom_element) {
+        hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, strAttributeName,
+                                     lFlags&ATTRFLAG_CASESENSITIVE ? fdexNameCaseSensitive : fdexNameCaseInsensitive, &dispid);
+        if(FAILED(hres)) {
+            V_VT(AttributeValue) = VT_NULL;
+            return (hres == DISP_E_UNKNOWNNAME) ? S_OK : hres;
+        }
 
-    hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, strAttributeName,
-            lFlags&ATTRFLAG_CASESENSITIVE ? fdexNameCaseSensitive : fdexNameCaseInsensitive, &dispid);
-    if(hres == DISP_E_UNKNOWNNAME) {
-        V_VT(AttributeValue) = VT_NULL;
-        return S_OK;
-    }
+        hres = get_elem_attr_value_by_dispid(This, dispid, AttributeValue);
+        if(FAILED(hres))
+            return hres;
 
-    if(FAILED(hres)) {
-        V_VT(AttributeValue) = VT_NULL;
+        if(lFlags & ATTRFLAG_ASSTRING)
+            hres = attr_value_to_string(AttributeValue);
         return hres;
     }
 
-    hres = get_elem_attr_value_by_dispid(This, dispid, AttributeValue);
-    if(SUCCEEDED(hres) && (lFlags & ATTRFLAG_ASSTRING))
-        hres = attr_value_to_string(AttributeValue);
-    return hres;
+    nsAString_InitDepend(&name_str, strAttributeName);
+    nsAString_InitDepend(&value_str, NULL);
+    nsres = nsIDOMElement_GetAttribute(This->dom_element, &name_str, &value_str);
+    nsAString_Finish(&name_str);
+    return return_nsstr_variant(nsres, &value_str, 0, AttributeValue);
 }
 
 static HRESULT WINAPI HTMLElement_removeAttribute(IHTMLElement *iface, BSTR strAttributeName,
-- 
2.31.1

[PATCH v12 2/7] mshtml: Restructure element getAttribute.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=103073

Your paranoid android.


=== debiant2 (32 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Chinese:China report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit WoW report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

[PATCH v12 2/7] mshtml: Restructure element getAttribute.

[Jacek Caban]

Signed-off-by: Jacek Caban <jacek(a)codeweavers.com>

[PATCH v12 3/7] mshtml: Restructure element removeAttribute.

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/mshtml/htmlelem.c | 55 +++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c
index b6c9db0..d718069 100644
--- a/dlls/mshtml/htmlelem.c
+++ b/dlls/mshtml/htmlelem.c
@@ -1197,46 +1197,47 @@ static HRESULT WINAPI HTMLElement_removeAttribute(IHTMLElement *iface, BSTR strA
                                                   LONG lFlags, VARIANT_BOOL *pfSuccess)
 {
     HTMLElement *This = impl_from_IHTMLElement(iface);
+    compat_mode_t compat_mode = dispex_compat_mode(&This->node.event_target.dispex);
     DISPID id;
     HRESULT hres;
 
     TRACE("(%p)->(%s %x %p)\n", This, debugstr_w(strAttributeName), lFlags, pfSuccess);
 
-    if(dispex_compat_mode(&This->node.event_target.dispex) >= COMPAT_MODE_IE8) {
-        *pfSuccess = element_has_attribute(This, strAttributeName);
-        if(*pfSuccess)
-            return element_remove_attribute(This, strAttributeName);
-        return S_OK;
-    }
+    if(compat_mode < COMPAT_MODE_IE8 || !This->dom_element) {
+        hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, strAttributeName,
+                                     lFlags&ATTRFLAG_CASESENSITIVE ? fdexNameCaseSensitive : fdexNameCaseInsensitive, &id);
+        if(hres == DISP_E_UNKNOWNNAME) {
+            *pfSuccess = VARIANT_FALSE;
+            return S_OK;
+        }
+        if(FAILED(hres))
+            return hres;
 
-    hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, strAttributeName,
-            lFlags&ATTRFLAG_CASESENSITIVE ? fdexNameCaseSensitive : fdexNameCaseInsensitive, &id);
-    if(hres == DISP_E_UNKNOWNNAME) {
-        *pfSuccess = VARIANT_FALSE;
-        return S_OK;
-    }
-    if(FAILED(hres))
-        return hres;
+        if(id == DISPID_IHTMLELEMENT_STYLE) {
+            IHTMLStyle *style;
 
-    if(id == DISPID_IHTMLELEMENT_STYLE) {
-        IHTMLStyle *style;
+            TRACE("Special case: style\n");
 
-        TRACE("Special case: style\n");
+            hres = IHTMLElement_get_style(&This->IHTMLElement_iface, &style);
+            if(FAILED(hres))
+                return hres;
 
-        hres = IHTMLElement_get_style(&This->IHTMLElement_iface, &style);
-        if(FAILED(hres))
-            return hres;
+            hres = IHTMLStyle_put_cssText(style, NULL);
+            IHTMLStyle_Release(style);
+            if(FAILED(hres))
+                return hres;
 
-        hres = IHTMLStyle_put_cssText(style, NULL);
-        IHTMLStyle_Release(style);
-        if(FAILED(hres))
-            return hres;
+            *pfSuccess = VARIANT_TRUE;
+            return S_OK;
+        }
 
-        *pfSuccess = VARIANT_TRUE;
-        return S_OK;
+        return remove_attribute(&This->node.event_target.dispex, id, pfSuccess);
     }
 
-    return remove_attribute(&This->node.event_target.dispex, id, pfSuccess);
+    *pfSuccess = element_has_attribute(This, strAttributeName);
+    if(*pfSuccess)
+        return element_remove_attribute(This, strAttributeName);
+    return S_OK;
 }
 
 static HRESULT WINAPI HTMLElement_put_className(IHTMLElement *iface, BSTR v)
-- 
2.31.1

[PATCH v12 3/7] mshtml: Restructure element removeAttribute.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=103074

Your paranoid android.


=== debiant2 (32 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Chinese:China report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

[PATCH v12 3/7] mshtml: Restructure element removeAttribute.

[Jacek Caban]

Signed-off-by: Jacek Caban <jacek(a)codeweavers.com>

[PATCH v12 4/7] mshtml: Associate props with attributes for IE8 mode, but keep them stringified.

[Gabriel Ivăncescu]

For non-builtin props, getAttribute retrieves the stringified value of the
prop. setAttribute also stringifies the value if it's a builtin only.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)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            | 83 +++++++++++++++++++++++++------
 dlls/mshtml/tests/documentmode.js | 77 ++++++++++++++++++++++------
 2 files changed, 129 insertions(+), 31 deletions(-)

diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c
index d718069..b776511 100644
--- a/dlls/mshtml/htmlelem.c
+++ b/dlls/mshtml/htmlelem.c
@@ -1067,6 +1067,16 @@ 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)
+{
+    static WCHAR classNameW[] = L"className";
+    WCHAR *ret = attr_name;
+
+    if(compat_mode >= COMPAT_MODE_IE8 && !wcsicmp(attr_name, L"class"))
+        ret = classNameW;
+    return ret;
+}
+
 static HRESULT set_elem_attr_value_by_dispid(HTMLElement *elem, DISPID dispid, VARIANT *v)
 {
     DISPID propput_dispid = DISPID_PROPERTYPUT;
@@ -1088,24 +1098,47 @@ static HRESULT WINAPI HTMLElement_setAttribute(IHTMLElement *iface, BSTR strAttr
     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(compat_mode < COMPAT_MODE_IE8 || !This->dom_element) {
-        hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, strAttributeName,
+    if(compat_mode < COMPAT_MODE_IE9 || !This->dom_element) {
+        hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, translate_attr_name(strAttributeName, compat_mode),
                 (lFlags&ATTRFLAG_CASESENSITIVE ? fdexNameCaseSensitive : fdexNameCaseInsensitive) | fdexNameEnsure, &dispid);
         if(FAILED(hres))
             return hres;
 
-        return set_elem_attr_value_by_dispid(This, dispid, &AttributeValue);
+        if(compat_mode >= COMPAT_MODE_IE8 && get_dispid_type(dispid) == DISPEXPROP_BUILTIN) {
+            if(V_VT(&val) != VT_BSTR && V_VT(&val) != VT_NULL) {
+                LCID lcid = MAKELCID(MAKELANGID(LANG_ENGLISH,SUBLANG_ENGLISH_US),SORT_DEFAULT);
+
+                V_VT(&val) = VT_EMPTY;
+                hres = VariantChangeTypeEx(&val, &AttributeValue, lcid, 0, VT_BSTR);
+                if(FAILED(hres))
+                    return hres;
+
+                if(V_BSTR(&val))
+                    needs_free = TRUE;
+                else
+                    V_VT(&val) = VT_NULL;
+            }
+        }
+
+        /* className and style are special cases */
+        if(compat_mode != COMPAT_MODE_IE8 || !This->dom_element ||
+           (dispid != DISPID_IHTMLELEMENT_CLASSNAME && dispid != DISPID_IHTMLELEMENT_STYLE)) {
+            hres = set_elem_attr_value_by_dispid(This, dispid, &val);
+            goto done;
+        }
     }
 
-    hres = variant_to_nsstr(&AttributeValue, FALSE, &value_str);
+    hres = variant_to_nsstr(&val, FALSE, &value_str);
     if(FAILED(hres))
-        return hres;
+        goto done;
 
     nsAString_InitDepend(&name_str, strAttributeName);
     nsres = nsIDOMElement_SetAttribute(This->dom_element, &name_str, &value_str);
@@ -1115,6 +1148,9 @@ static HRESULT WINAPI HTMLElement_setAttribute(IHTMLElement *iface, BSTR strAttr
         WARN("SetAttribute failed: %08x\n", nsres);
     hres = map_nsresult(nsres);
 
+done:
+    if(needs_free)
+        SysFreeString(V_BSTR(&val));
     return hres;
 }
 
@@ -1169,21 +1205,35 @@ static HRESULT WINAPI HTMLElement_getAttribute(IHTMLElement *iface, BSTR strAttr
     if(lFlags & ~(ATTRFLAG_CASESENSITIVE|ATTRFLAG_ASSTRING))
         FIXME("Unsupported flags %x\n", lFlags);
 
-    if(compat_mode < COMPAT_MODE_IE8 || !This->dom_element) {
-        hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, strAttributeName,
+    if(compat_mode < COMPAT_MODE_IE9 || !This->dom_element) {
+        hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, translate_attr_name(strAttributeName, compat_mode),
                                      lFlags&ATTRFLAG_CASESENSITIVE ? fdexNameCaseSensitive : fdexNameCaseInsensitive, &dispid);
         if(FAILED(hres)) {
             V_VT(AttributeValue) = VT_NULL;
             return (hres == DISP_E_UNKNOWNNAME) ? S_OK : hres;
         }
 
-        hres = get_elem_attr_value_by_dispid(This, dispid, AttributeValue);
-        if(FAILED(hres))
-            return hres;
+        /* className and style are special cases */
+        if(compat_mode != COMPAT_MODE_IE8 || !This->dom_element ||
+           (dispid != DISPID_IHTMLELEMENT_CLASSNAME && dispid != DISPID_IHTMLELEMENT_STYLE)) {
+            hres = get_elem_attr_value_by_dispid(This, dispid, AttributeValue);
+            if(FAILED(hres))
+                return hres;
 
-        if(lFlags & ATTRFLAG_ASSTRING)
-            hres = attr_value_to_string(AttributeValue);
-        return hres;
+            if(compat_mode >= COMPAT_MODE_IE8 && V_VT(AttributeValue) != VT_BSTR && V_VT(AttributeValue) != VT_NULL) {
+                LCID lcid = MAKELCID(MAKELANGID(LANG_ENGLISH,SUBLANG_ENGLISH_US),SORT_DEFAULT);
+
+                hres = VariantChangeTypeEx(AttributeValue, AttributeValue, lcid, 0, VT_BSTR);
+                if(FAILED(hres)) {
+                    VariantClear(AttributeValue);
+                    return hres;
+                }
+                if(!V_BSTR(AttributeValue))
+                    V_VT(AttributeValue) = VT_NULL;
+            }else if(lFlags & ATTRFLAG_ASSTRING)
+                hres = attr_value_to_string(AttributeValue);
+            return hres;
+        }
     }
 
     nsAString_InitDepend(&name_str, strAttributeName);
@@ -1203,8 +1253,8 @@ static HRESULT WINAPI HTMLElement_removeAttribute(IHTMLElement *iface, BSTR strA
 
     TRACE("(%p)->(%s %x %p)\n", This, debugstr_w(strAttributeName), lFlags, pfSuccess);
 
-    if(compat_mode < COMPAT_MODE_IE8 || !This->dom_element) {
-        hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, strAttributeName,
+    if(compat_mode < COMPAT_MODE_IE9 || !This->dom_element) {
+        hres = IDispatchEx_GetDispID(&This->node.event_target.dispex.IDispatchEx_iface, translate_attr_name(strAttributeName, compat_mode),
                                      lFlags&ATTRFLAG_CASESENSITIVE ? fdexNameCaseSensitive : fdexNameCaseInsensitive, &id);
         if(hres == DISP_E_UNKNOWNNAME) {
             *pfSuccess = VARIANT_FALSE;
@@ -1231,7 +1281,8 @@ static HRESULT WINAPI HTMLElement_removeAttribute(IHTMLElement *iface, BSTR strA
             return S_OK;
         }
 
-        return remove_attribute(&This->node.event_target.dispex, id, pfSuccess);
+        if(compat_mode != COMPAT_MODE_IE8 || !This->dom_element || id != DISPID_IHTMLELEMENT_CLASSNAME)
+            return remove_attribute(&This->node.event_target.dispex, id, pfSuccess);
     }
 
     *pfSuccess = element_has_attribute(This, strAttributeName);
diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js
index c2fd8bd..cf3fdfa 100644
--- a/dlls/mshtml/tests/documentmode.js
+++ b/dlls/mshtml/tests/documentmode.js
@@ -518,9 +518,7 @@ sync_test("createElement_inline_attr", function() {
         for(var i = 0; i < tags.length; i++) {
             e = document.createElement("<" + tags[i] + " test='a\"' abcd=\""b"\">");
             ok(e.tagName === tags[i].toUpperCase(), "<" + tags[i] + " test=\"a\" abcd=\"b\">.tagName returned " + e.tagName);
-            todo_wine_if(v == 8).
             ok(e.test === "a\"", "<" + tags[i] + " test='a\"' abcd=\""b"\">.test returned " + e.test);
-            todo_wine_if(v == 8).
             ok(e.abcd === "\"b\"", "<" + tags[i] + " test='a\"' abcd=\""b"\">.abcd returned " + e.abcd);
         }
     }else {
@@ -1063,6 +1061,13 @@ sync_test("elem_attr", function() {
     var v = document.documentMode;
     var elem = document.createElement("div"), r;
 
+    function test_exposed(prop, expect) {
+        if(expect)
+            ok(prop in elem, prop + " is not exposed from elem");
+        else
+            ok(!(prop in elem), prop + " is exposed from elem");
+    }
+
     r = elem.getAttribute("class");
     ok(r === null, "class attr = " + r);
     r = elem.getAttribute("className");
@@ -1088,11 +1093,37 @@ sync_test("elem_attr", function() {
     r = elem.getAttribute("className");
     ok(r === "cls3", "className attr = " + r);
 
+    elem.htmlFor = "for";
+    r = elem.getAttribute("for");
+    ok(r === null, "for attr = " + r);
+    r = elem.getAttribute("htmlFor");
+    ok(r === (v < 9 ? "for" : null), "htmlFor attr = " + r);
+
+    elem.setAttribute("for", "for2");
+    ok(elem.htmlFor === "for", "elem.htmlFor = " + elem.htmlFor);
+    r = elem.getAttribute("for");
+    ok(r === "for2", "for attr = " + r);
+    r = elem.getAttribute("htmlFor");
+    ok(r === (v < 9 ? "for" : null), "htmlFor attr = " + r);
+
+    elem.setAttribute("htmlFor", "for3");
+    ok(elem.htmlFor === (v < 9 ? "for3" : "for"), "elem.htmlFor = " + elem.htmlFor);
+    r = elem.getAttribute("for");
+    ok(r === "for2", "for attr = " + r);
+    r = elem.getAttribute("htmlFor");
+    ok(r === "for3", "htmlFor attr = " + r);
+
+    elem.setAttribute("testattr", "test");
+    test_exposed("class", v < 8);
+    test_exposed("className", true);
+    test_exposed("for", v < 9);
+    test_exposed("htmlFor", true);
+    test_exposed("testattr", v < 9);
+
     var arr = [3];
     elem.setAttribute("testattr", arr);
     r = elem.getAttribute("testattr");
     ok(r === (v < 8 ? arr : "3"), "testattr = " + r);
-    todo_wine_if(v === 8).
     ok(elem.testattr === (v < 9 ? arr : undefined), "elem.testattr = " + elem.testattr);
     r = elem.removeAttribute("testattr");
     ok(r === (v < 9 ? true : undefined), "testattr removeAttribute returned " + r);
@@ -1102,23 +1133,19 @@ sync_test("elem_attr", function() {
     elem.setAttribute("testattr", "string");
     elem.testattr = arr;
     r = elem.getAttribute("testattr");
-    todo_wine_if(v === 8).
     ok(r === (v < 8 ? arr : (v < 9 ? "9" : "string")), "testattr = " + r);
     ok(elem.testattr === arr, "elem.testattr = " + elem.testattr);
     arr[0] = 3;
     r = elem.getAttribute("testattr");
-    todo_wine_if(v === 8).
     ok(r === (v < 8 ? arr : (v < 9 ? "3" : "string")), "testattr = " + r);
     ok(elem.testattr === arr, "elem.testattr = " + elem.testattr);
     r = elem.removeAttribute("testattr");
     ok(r === (v < 9 ? true : undefined), "testattr removeAttribute returned " + r);
-    todo_wine_if(v === 8).
     ok(elem.testattr === (v < 9 ? undefined : arr), "removed testattr = " + elem.testattr);
 
     arr.toString = function() { return 42; }
     elem.testattr = arr;
     r = elem.getAttribute("testattr");
-    todo_wine_if(v === 8).
     ok(r === (v < 8 ? arr : (v < 9 ? "42" : null)), "testattr with custom toString = " + r);
     elem.setAttribute("testattr", arr);
     r = elem.getAttribute("testattr");
@@ -1126,13 +1153,11 @@ sync_test("elem_attr", function() {
     ok(elem.testattr === arr, "elem.testattr after setAttribute with custom toString = " + elem.testattr);
     r = elem.removeAttribute("testattr");
     ok(r === (v < 9 ? true : undefined), "testattr removeAttribute with custom toString returned " + r);
-    todo_wine_if(v === 8).
     ok(elem.testattr === (v < 9 ? undefined : arr), "removed testattr with custom toString = " + elem.testattr);
 
     arr.valueOf = function() { return "arrval"; }
     elem.testattr = arr;
     r = elem.getAttribute("testattr");
-    todo_wine_if(v === 8).
     ok(r === (v < 8 ? arr : (v < 9 ? "arrval" : null)), "testattr with custom valueOf = " + r);
     elem.setAttribute("testattr", arr);
     r = elem.getAttribute("testattr");
@@ -1140,7 +1165,6 @@ sync_test("elem_attr", function() {
     ok(elem.testattr === arr, "elem.testattr after setAttribute with custom valueOf = " + elem.testattr);
     r = elem.removeAttribute("testattr");
     ok(r === (v < 9 ? true : undefined), "testattr removeAttribute with custom valueOf returned " + r);
-    todo_wine_if(v === 8).
     ok(elem.testattr === (v < 9 ? undefined : arr), "removed testattr with custom valueOf = " + elem.testattr);
     delete arr.valueOf;
     delete arr.toString;
@@ -1157,6 +1181,7 @@ sync_test("elem_attr", function() {
     elem.onclick = func;
     ok(elem.onclick === func, "onclick = " + elem.onclick);
     r = elem.getAttribute("onclick");
+    todo_wine_if(v === 8).
     ok(r === (v < 8 ? func : null), "onclick attr = " + r);
     r = elem.removeAttribute("onclick");
     ok(r === (v < 9 ? false : undefined), "removeAttribute returned " + r);
@@ -1166,7 +1191,6 @@ sync_test("elem_attr", function() {
     elem.onclick_test = func;
     ok(elem.onclick_test === func, "onclick_test = " + elem.onclick_test);
     r = elem.getAttribute("onclick_test");
-    todo_wine_if(v === 8).
     ok(r === (v < 8 ? func : (v < 9 ? func.toString() : null)), "onclick_test attr = " + r);
 
     elem.setAttribute("onclick", "test");
@@ -1176,11 +1200,11 @@ sync_test("elem_attr", function() {
     ok(r === (v < 9 ? true : undefined), "removeAttribute after setAttribute returned " + r);
 
     /* IE11 returns an empty function, which we can't check directly */
-    todo_wine_if(v >= 8).
+    todo_wine_if(v >= 9).
     ok((v < 11) ? (elem.onclick === null) : (elem.onclick !== func), "removed onclick after setAttribute = " + elem.onclick);
 
     r = Object.prototype.toString.call(elem.onclick);
-    todo_wine_if(v >= 8 && v < 11).
+    todo_wine_if(v >= 9 && v < 11).
     ok(r === (v < 9 ? "[object Object]" : (v < 11 ? "[object Null]" : "[object Function]")),
         "removed onclick after setAttribute Object.toString returned " + r);
 
@@ -1194,11 +1218,10 @@ sync_test("elem_attr", function() {
     ok(r === (v < 8 ? func : (v < 9 ? null : "string")), "onclick attr = " + r);
     elem.onclick = "test";
     r = elem.getAttribute("onclick");
-    todo_wine_if(v === 8).
     ok(r === (v < 9 ? "test" : "string"), "onclick attr = " + r);
     r = elem.removeAttribute("onclick");
     ok(r === (v < 9 ? true : undefined), "removeAttribute returned " + r);
-    todo_wine_if(v >= 8).
+    todo_wine_if(v >= 9).
     ok(elem.onclick === null, "removed onclick = " + elem.onclick);
 
     elem.setAttribute("ondblclick", arr);
@@ -1218,6 +1241,30 @@ sync_test("elem_attr", function() {
     r = elem.removeAttribute("ondblclick");
     ok(r === (v < 9 ? true : undefined), "ondblclick string removeAttribute returned " + r);
     ok(elem.ondblclick === null, "removed ondblclick string = " + elem.ondblclick);
+
+    if(v < 9) {
+        /* style is a special case */
+        try {
+            elem.style = "opacity: 1.0";
+            ok(false, "expected exception setting elem.style");
+        }catch(ex) { }
+
+        var style = elem.style;
+        r = elem.getAttribute("style");
+        ok(r === (v < 8 ? style : null), "style attr = " + r);
+        r = elem.removeAttribute("style");
+        ok(r === true, "removeAttribute('style') returned " + r);
+        r = elem.style;
+        ok(r === style, "removed elem.style = " + r);
+        r = elem.getAttribute("style");
+        todo_wine_if(v === 8).
+        ok(r === (v < 8 ? style : null), "style attr after removal = " + r);
+        elem.setAttribute("style", "opacity: 1.0");
+        r = elem.getAttribute("style");
+        ok(r === (v < 8 ? style : "opacity: 1.0"), "style attr after setAttribute = " + r);
+        r = elem.style;
+        ok(r === style, "elem.style after setAttribute = " + r);
+    }
 });
 
 sync_test("__proto__", function() {
-- 
2.31.1

[PATCH v12 4/7] mshtml: Associate props with attributes for IE8 mode, but keep them stringified.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=103075

Your paranoid android.


=== w8adm (32 bit report) ===

mshtml:
events.c:1089: Test failed: unexpected call img_onerror
events: Timeout

=== debiant2 (32 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Arabic:Morocco report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit German report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit French report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Hebrew:Israel report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Hindi:India report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Japanese:Japan report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Chinese:China report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit WoW report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

[PATCH v12 4/7] mshtml: Associate props with attributes for IE8 mode, but keep them stringified.

[Jacek Caban]

Signed-off-by: Jacek Caban <jacek(a)codeweavers.com>

[PATCH v12 5/7] mshtml: Fix style attribute removal in IE8 mode.

[Gabriel Ivăncescu]

put_cssText implicitly sets the attribute, which must be removed since IE8
specially checks it.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/mshtml/htmlelem.c            | 3 +++
 dlls/mshtml/tests/documentmode.js | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c
index b776511..862b0eb 100644
--- a/dlls/mshtml/htmlelem.c
+++ b/dlls/mshtml/htmlelem.c
@@ -1277,6 +1277,9 @@ static HRESULT WINAPI HTMLElement_removeAttribute(IHTMLElement *iface, BSTR strA
             if(FAILED(hres))
                 return hres;
 
+            if(compat_mode >= COMPAT_MODE_IE8)
+                element_remove_attribute(This, strAttributeName);
+
             *pfSuccess = VARIANT_TRUE;
             return S_OK;
         }
diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js
index cf3fdfa..2564186 100644
--- a/dlls/mshtml/tests/documentmode.js
+++ b/dlls/mshtml/tests/documentmode.js
@@ -1257,7 +1257,6 @@ sync_test("elem_attr", function() {
         r = elem.style;
         ok(r === style, "removed elem.style = " + r);
         r = elem.getAttribute("style");
-        todo_wine_if(v === 8).
         ok(r === (v < 8 ? style : null), "style attr after removal = " + r);
         elem.setAttribute("style", "opacity: 1.0");
         r = elem.getAttribute("style");
-- 
2.31.1

[PATCH v12 5/7] mshtml: Fix style attribute removal in IE8 mode.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=103076

Your paranoid android.


=== w8adm (32 bit report) ===

mshtml:
events.c:1089: Test failed: unexpected call img_onerror
events: Timeout

=== w8 (32 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:350: Test failed: expected Exec_SETTITLE
htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE

=== w8adm (32 bit report) ===

mshtml:
htmldoc.c:3084: Test failed: Incorrect error code: -2146697211
htmldoc.c:3089: Test failed: Page address: L"http://test.winehq.org/tests/winehq_snapshot/"
htmldoc.c:5861: Test failed: expected OnChanged_1012
htmldoc.c:5862: Test failed: expected Exec_HTTPEQUIV
htmldoc.c:5864: Test failed: expected Exec_SETTITLE
htmldoc.c:5905: Test failed: expected FireNavigateComplete2

=== w10pro64_ja (64 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== w10pro64_zh_CN (64 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Arabic:Morocco report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit German report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit French report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Hebrew:Israel report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Hindi:India report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Japanese:Japan report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Chinese:China report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit WoW report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

[PATCH v12 5/7] mshtml: Fix style attribute removal in IE8 mode.

[Jacek Caban]

Signed-off-by: Jacek Caban <jacek(a)codeweavers.com>

[PATCH v12 6/7] mshtml: Set the builtin prop to NULL even when removeAttribute returns false in IE8 mode.

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/mshtml/dispex.c              | 8 ++++++--
 dlls/mshtml/tests/documentmode.js | 1 -
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c
index 9f56a56..e473d7b 100644
--- a/dlls/mshtml/dispex.c
+++ b/dlls/mshtml/dispex.c
@@ -1470,9 +1470,13 @@ HRESULT remove_attribute(DispatchEx *This, DISPID id, VARIANT_BOOL *success)
         if(FAILED(hres)) {
             VARIANT *ref;
             hres = dispex_get_dprop_ref(This, func->name, FALSE, &ref);
-            if(FAILED(hres) || V_VT(ref) != VT_BSTR)
+            if(FAILED(hres) || V_VT(ref) != VT_BSTR) {
                 *success = VARIANT_FALSE;
-            else
+                if(dispex_compat_mode(This) >= COMPAT_MODE_IE8) {
+                    V_VT(&var) = VT_NULL;
+                    builtin_propput(This, func, &dp, NULL);
+                }
+            }else
                 VariantClear(ref);
         }
         return S_OK;
diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js
index 2564186..63c0df5 100644
--- a/dlls/mshtml/tests/documentmode.js
+++ b/dlls/mshtml/tests/documentmode.js
@@ -1185,7 +1185,6 @@ sync_test("elem_attr", function() {
     ok(r === (v < 8 ? func : null), "onclick attr = " + r);
     r = elem.removeAttribute("onclick");
     ok(r === (v < 9 ? false : undefined), "removeAttribute returned " + r);
-    todo_wine_if(v === 8).
     ok(elem.onclick === (v != 8 ? func : null), "removed onclick = " + elem.onclick);
 
     elem.onclick_test = func;
-- 
2.31.1

[PATCH v12 6/7] mshtml: Set the builtin prop to NULL even when removeAttribute returns false in IE8 mode.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=103077

Your paranoid android.


=== w8 (32 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:350: Test failed: expected Exec_SETTITLE
htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE

=== w10pro64_zh_CN (64 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== w7u_adm (32 bit report) ===

mshtml:
script.c:624: Test failed: L"/index.html?es5.js:date_now: unexpected Date.now() result 1638373884976 expected 1638373885039"

=== debiant2 (32 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Arabic:Morocco report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit German report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit French report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Hindi:India report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Japanese:Japan report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Chinese:China report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

[PATCH v12 6/7] mshtml: Set the builtin prop to NULL even when removeAttribute returns false in IE8 mode.

[Jacek Caban]

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

[PATCH v12 6/7] mshtml: Set the builtin prop to NULL even when removeAttribute returns false in IE8 mode.

[Gabriel Ivăncescu]

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

[PATCH v12 6/7] mshtml: Set the builtin prop to NULL even when removeAttribute returns false in IE8 mode.

[Jacek Caban]

On 12/2/21 2:20 PM, Gabriel Ivăncescu wrote:
> 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.

Yes, the existing code is already wrong, we probably shouldn't blindly 
try to set value to VT_EMPTY in the first place. Trying to set the 
property to another value (of type that doesn't make sense unless you 
assume that those are event handlers) based on a previous error that we 
know nothing about (it could be OOM?) is making it even worse. Is there 
another reason for this patch than a todo_wine in tests?


Thanks,

Jacek

[PATCH v12 6/7] mshtml: Set the builtin prop to NULL even when removeAttribute returns false in IE8 mode.

[Gabriel Ivăncescu]

On 02/12/2021 15:51, Jacek Caban wrote:
> On 12/2/21 2:20 PM, Gabriel Ivăncescu wrote:
> > 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.
>
> Yes, the existing code is already wrong, we probably shouldn't blindly 
> try to set value to VT_EMPTY in the first place. Trying to set the 
> property to another value (of type that doesn't make sense unless you 
> assume that those are event handlers) based on a previous error that we 
> know nothing about (it could be OOM?) is making it even worse. Is there 
> another reason for this patch than a todo_wine in tests?
>
> Thanks,
>
> Jacek

I think the VT_EMPTY is correct for pre IE8 modes, since I didn't get 
any failures there, just not for IE8.

Even if we assume that we do implement it correctly for IE8 to reset it 
to default values at some point, it would *still* not work without this 
patch for this particular case, since that would handle the case where 
it returns true, not false. Isn't this patch still needed, then? I can't 
see how else it would work.

As for the reason, well, prototype.js sets "onclick" to an array and 
later removeAttribute on it. Not sure to what extent it's useful though. 
But it was a pretty simple fix that I honestly see no downsides to—even 
in the future should we implement the rest properly.

[PATCH v12 6/7] mshtml: Set the builtin prop to NULL even when removeAttribute returns false in IE8 mode.

[Jacek Caban]

On 12/2/21 4:37 PM, Gabriel Ivăncescu wrote:
> On 02/12/2021 15:51, Jacek Caban wrote:
> > On 12/2/21 2:20 PM, Gabriel Ivăncescu wrote:
> > > 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.
> >
> > Yes, the existing code is already wrong, we probably shouldn't 
> > blindly try to set value to VT_EMPTY in the first place. Trying to 
> > set the property to another value (of type that doesn't make sense 
> > unless you assume that those are event handlers) based on a previous 
> > error that we know nothing about (it could be OOM?) is making it even 
> > worse. Is there another reason for this patch than a todo_wine in tests?
> >
> > Thanks,
> >
> > Jacek
>
> I think the VT_EMPTY is correct for pre IE8 modes, since I didn't get 
> any failures there, just not for IE8.
>
> Even if we assume that we do implement it correctly for IE8 to reset 
> it to default values at some point, it would *still* not work without 
> this patch for this particular case, since that would handle the case 
> where it returns true, not false. Isn't this patch still needed, then? 
> I can't see how else it would work.

I would need a deeper look to propose a solution. For example, if we had 
a way to recognize an event handler property, it could help here.
> As for the reason, well, prototype.js sets "onclick" to an array and 
> later removeAttribute on it. Not sure to what extent it's useful 
> though. But it was a pretty simple fix that I honestly see no 
> downsides to—even in the future should we implement the rest properly.

Well, it's really not that hard to read prototype.js. It just does a 
check if attributes are converted to string and if not, applies some 
hacks for early IE version soon. removeAttribute is just a clean up, it 
will take the right code path anyway. And whatever code path it takes, 
it should be fine for us.


Jacek

[PATCH v12 7/7] mshtml: Set dispatch values to "[object]" for builtin attributes in IE8 and IE9 modes.

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/mshtml/dispex.c              |  2 +-
 dlls/mshtml/htmlelem.c            | 16 ++++++++++++++--
 dlls/mshtml/mshtml_private.h      |  1 +
 dlls/mshtml/tests/documentmode.js |  2 --
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c
index e473d7b..7ac98e6 100644
--- a/dlls/mshtml/dispex.c
+++ b/dlls/mshtml/dispex.c
@@ -1007,7 +1007,7 @@ static HRESULT get_builtin_func(dispex_data_t *data, DISPID id, func_info_t **re
     return DISP_E_UNKNOWNNAME;
 }
 
-static HRESULT get_builtin_id(DispatchEx *This, BSTR name, DWORD grfdex, DISPID *ret)
+HRESULT get_builtin_id(DispatchEx *This, BSTR name, DWORD grfdex, DISPID *ret)
 {
     int min, max, n, c;
 
diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c
index 862b0eb..e2da83c 100644
--- a/dlls/mshtml/htmlelem.c
+++ b/dlls/mshtml/htmlelem.c
@@ -1100,9 +1100,9 @@ static HRESULT WINAPI HTMLElement_setAttribute(IHTMLElement *iface, BSTR strAttr
     nsAString name_str, value_str;
     VARIANT val = AttributeValue;
     BOOL needs_free = FALSE;
+    HRESULT hres = S_OK;
     nsresult nsres;
     DISPID dispid;
-    HRESULT hres;
 
     TRACE("(%p)->(%s %s %08x)\n", This, debugstr_w(strAttributeName), debugstr_variant(&AttributeValue), lFlags);
 
@@ -1113,7 +1113,12 @@ static HRESULT WINAPI HTMLElement_setAttribute(IHTMLElement *iface, BSTR strAttr
             return hres;
 
         if(compat_mode >= COMPAT_MODE_IE8 && get_dispid_type(dispid) == DISPEXPROP_BUILTIN) {
-            if(V_VT(&val) != VT_BSTR && V_VT(&val) != VT_NULL) {
+            if(V_VT(&val) == VT_DISPATCH && compat_mode < COMPAT_MODE_IE10) {
+                if(!(V_BSTR(&val) = SysAllocString(L"[object]")))
+                    return E_OUTOFMEMORY;
+                V_VT(&val) = VT_BSTR;
+                needs_free = TRUE;
+            }else if(V_VT(&val) != VT_BSTR && V_VT(&val) != VT_NULL) {
                 LCID lcid = MAKELCID(MAKELANGID(LANG_ENGLISH,SUBLANG_ENGLISH_US),SORT_DEFAULT);
 
                 V_VT(&val) = VT_EMPTY;
@@ -1134,6 +1139,13 @@ static HRESULT WINAPI HTMLElement_setAttribute(IHTMLElement *iface, BSTR strAttr
             hres = set_elem_attr_value_by_dispid(This, dispid, &val);
             goto done;
         }
+    }else if(compat_mode < COMPAT_MODE_IE10 && V_VT(&val) == VT_DISPATCH) {
+        if(get_builtin_id(&This->node.event_target.dispex, strAttributeName, fdexNameCaseInsensitive, &dispid) == S_OK) {
+            if(!(V_BSTR(&val) = SysAllocString(L"[object]")))
+                return E_OUTOFMEMORY;
+            V_VT(&val) = VT_BSTR;
+            needs_free = TRUE;
+        }
     }
 
     hres = variant_to_nsstr(&val, FALSE, &value_str);
diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h
index 6978ed7..86ea21e 100644
--- a/dlls/mshtml/mshtml_private.h
+++ b/dlls/mshtml/mshtml_private.h
@@ -385,6 +385,7 @@ void init_dispatch(DispatchEx*,IUnknown*,dispex_static_data_t*,compat_mode_t) DE
 void release_dispex(DispatchEx*) DECLSPEC_HIDDEN;
 BOOL dispex_query_interface(DispatchEx*,REFIID,void**) DECLSPEC_HIDDEN;
 HRESULT change_type(VARIANT*,VARIANT*,VARTYPE,IServiceProvider*) DECLSPEC_HIDDEN;
+HRESULT get_builtin_id(DispatchEx*,BSTR,DWORD,DISPID*) DECLSPEC_HIDDEN;
 HRESULT dispex_get_dprop_ref(DispatchEx*,const WCHAR*,BOOL,VARIANT**) DECLSPEC_HIDDEN;
 HRESULT get_dispids(tid_t,DWORD*,DISPID**) DECLSPEC_HIDDEN;
 HRESULT remove_attribute(DispatchEx*,DISPID,VARIANT_BOOL*) DECLSPEC_HIDDEN;
diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js
index 63c0df5..6190d36 100644
--- a/dlls/mshtml/tests/documentmode.js
+++ b/dlls/mshtml/tests/documentmode.js
@@ -1171,7 +1171,6 @@ sync_test("elem_attr", function() {
 
     elem.setAttribute("id", arr);
     r = elem.getAttribute("id");
-    todo_wine_if(v >= 8 && v < 10).
     ok(r === (v < 8 || v >= 10 ? "3" : "[object]"), "id = " + r);
     r = elem.removeAttribute("id");
     ok(r === (v < 9 ? true : undefined), "id removeAttribute returned " + r);
@@ -1225,7 +1224,6 @@ sync_test("elem_attr", function() {
 
     elem.setAttribute("ondblclick", arr);
     r = elem.getAttribute("ondblclick");
-    todo_wine_if(v >= 8 && v < 10).
     ok(r === (v < 8 ? arr : (v < 10 ? "[object]" : "3")), "ondblclick = " + r);
     r = elem.removeAttribute("ondblclick");
     ok(r === (v < 8 ? false : (v < 9 ? true : undefined)), "ondblclick removeAttribute returned " + r);
-- 
2.31.1

[PATCH v12 7/7] mshtml: Set dispatch values to "[object]" for builtin attributes in IE8 and IE9 modes.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=103078

Your paranoid android.


=== w10pro64 (32 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Arabic:Morocco report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit German report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit French report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Hebrew:Israel report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Hindi:India report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Japanese:Japan report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Chinese:China report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit WoW report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=103072

Your paranoid android.


=== debiant2 (32 bit report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

=== debiant2 (32 bit Chinese:China report) ===

mshtml:
htmldoc.c:2541: Test failed: unexpected call UpdateUI
htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

[Jacek Caban]

Signed-off-by: Jacek Caban <jacek(a)codeweavers.com>