[PATCH v4 0/6] MR8251: mshtml: getAttributeNode and removal fixes.
-- v4: mshtml: Implement 'specified' for detached attributes. mshtml: Detach attribute nodes when removing the attribute from the element. mshtml: Use a helper function to find an attribute in the collection's list. mshtml: Use a BSTR to store the detached attribute's name. mshtml: Clone name properly from attached attribute nodes. mshtml: Use HasAttribute instead of GetAttributeNode when checking if https://gitlab.winehq.org/wine/wine/-/merge_requests/8251
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlattr.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/dlls/mshtml/htmlattr.c b/dlls/mshtml/htmlattr.c index 1a95c1b482e..ff6173aacdb 100644 --- a/dlls/mshtml/htmlattr.c +++ b/dlls/mshtml/htmlattr.c @@ -87,9 +87,9 @@ static HRESULT WINAPI HTMLDOMAttribute_get_nodeValue(IHTMLDOMAttribute *iface, V static HRESULT WINAPI HTMLDOMAttribute_get_specified(IHTMLDOMAttribute *iface, VARIANT_BOOL *p) { HTMLDOMAttribute *This = impl_from_IHTMLDOMAttribute(iface); - nsIDOMAttr *nsattr; nsAString nsname; nsresult nsres; + cpp_bool r; TRACE("(%p)->(%p)\n", This, p); @@ -105,19 +105,10 @@ static HRESULT WINAPI HTMLDOMAttribute_get_specified(IHTMLDOMAttribute *iface, V /* FIXME: This is not exactly right, we have some attributes that don't map directly to Gecko attributes. */ nsAString_InitDepend(&nsname, dispex_builtin_prop_name(&This->elem->node.event_target.dispex, This->dispid)); - nsres = nsIDOMElement_GetAttributeNode(This->elem->dom_element, &nsname, &nsattr); + nsres = nsIDOMElement_HasAttribute(This->elem->dom_element, &nsname, &r); nsAString_Finish(&nsname); - if(NS_FAILED(nsres)) - return E_FAIL; - /* If the Gecko attribute node can be found, we know that the attribute is specified. - There is no point in calling GetSpecified */ - if(nsattr) { - nsIDOMAttr_Release(nsattr); - *p = VARIANT_TRUE; - }else { - *p = VARIANT_FALSE; - } + *p = variant_bool(NS_SUCCEEDED(nsres) && r); return S_OK; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8251
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlattr.c | 21 +++++++++++++++------ dlls/mshtml/tests/dom.c | 10 +++++++++- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/dlls/mshtml/htmlattr.c b/dlls/mshtml/htmlattr.c index ff6173aacdb..82d6aaf9476 100644 --- a/dlls/mshtml/htmlattr.c +++ b/dlls/mshtml/htmlattr.c @@ -344,17 +344,26 @@ static HRESULT WINAPI HTMLDOMAttribute2_cloneNode(IHTMLDOMAttribute2 *iface, VAR HTMLDOMAttribute *This = impl_from_IHTMLDOMAttribute2(iface); HTMLDOMAttribute *new_attr; HRESULT hres; + BSTR name; TRACE("(%p)->(%x %p)\n", This, fDeep, clonedNode); - hres = HTMLDOMAttribute_Create(This->name, NULL, 0, This->doc, &new_attr); - if(FAILED(hres)) - return hres; - - if(This->elem) + if(This->elem) { + hres = dispex_prop_name(&This->elem->node.event_target.dispex, This->dispid, &name); + if(FAILED(hres)) + return hres; + hres = HTMLDOMAttribute_Create(name, NULL, 0, This->doc, &new_attr); + SysFreeString(name); + if(FAILED(hres)) + return hres; hres = get_elem_attr_value_by_dispid(This->elem, This->dispid, &new_attr->value); - else + }else { + hres = HTMLDOMAttribute_Create(This->name, NULL, 0, This->doc, &new_attr); + if(FAILED(hres)) + return hres; hres = VariantCopy(&new_attr->value, &This->value); + } + if(FAILED(hres)) { IHTMLDOMAttribute_Release(&new_attr->IHTMLDOMAttribute_iface); return hres; diff --git a/dlls/mshtml/tests/dom.c b/dlls/mshtml/tests/dom.c index f0ac1053c40..9b52f7290c7 100644 --- a/dlls/mshtml/tests/dom.c +++ b/dlls/mshtml/tests/dom.c @@ -10093,9 +10093,9 @@ static void test_attr_node(IHTMLDOMAttribute *test_attr, IHTMLDocument2 *doc) IHTMLElement *elem; VARIANT_BOOL vbool; VARIANT v, v_clone; + BSTR bstr, bstr2; IDispatch *disp; HRESULT hres; - BSTR bstr; LONG type; hres = IHTMLDOMAttribute_QueryInterface(test_attr, &IID_IHTMLDOMAttribute2, (void**)&attr); @@ -10127,6 +10127,14 @@ static void test_attr_node(IHTMLDOMAttribute *test_attr, IHTMLDocument2 *doc) ok(hres == S_OK, "cloneNode failed: %08lx\n", hres); ok(!iface_cmp((IUnknown*)attr, (IUnknown*)clone), "attr == cloned attr\n"); + hres = IHTMLDOMAttribute_get_nodeName(test_attr, &bstr); + ok(hres == S_OK, "get_nodeName failed: %08lx\n", hres); + hres = IHTMLDOMAttribute_get_nodeName(clone, &bstr2); + ok(hres == S_OK, "get_nodeName failed: %08lx\n", hres); + ok(!wcscmp(bstr, bstr2), "attr name %s != cloned attr name %s\n", wine_dbgstr_w(bstr), wine_dbgstr_w(bstr2)); + SysFreeString(bstr2); + SysFreeString(bstr); + V_VT(&v) = VT_EMPTY; V_VT(&v_clone) = VT_EMPTY; hres = IHTMLDOMAttribute_get_nodeValue(test_attr, &v); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8251
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlattr.c | 6 +++--- dlls/mshtml/mshtml_private.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/dlls/mshtml/htmlattr.c b/dlls/mshtml/htmlattr.c index 82d6aaf9476..a83298a7ea1 100644 --- a/dlls/mshtml/htmlattr.c +++ b/dlls/mshtml/htmlattr.c @@ -51,7 +51,7 @@ static HRESULT WINAPI HTMLDOMAttribute_get_nodeName(IHTMLDOMAttribute *iface, BS return E_FAIL; } - *p = SysAllocString(This->name); + *p = SysAllocStringLen(This->name, SysStringLen(This->name)); return *p ? S_OK : E_OUTOFMEMORY; } @@ -537,7 +537,7 @@ static void HTMLDOMAttribute_destructor(DispatchEx *dispex) { HTMLDOMAttribute *This = impl_from_DispatchEx(dispex); VariantClear(&This->value); - free(This->name); + SysFreeString(This->name); free(This); } @@ -609,7 +609,7 @@ HRESULT HTMLDOMAttribute_Create(const WCHAR *name, HTMLElement *elem, DISPID dis /* For detached attributes we may still do most operations if we have its name available. */ if(name) { - ret->name = wcsdup(name); + ret->name = SysAllocString(name); if(!ret->name) { IHTMLDOMAttribute_Release(&ret->IHTMLDOMAttribute_iface); return E_OUTOFMEMORY; diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index 4319c8b4725..ca49312fbe3 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -1341,7 +1341,7 @@ typedef struct { /* value is valid only for detached attributes (when elem == NULL). */ VARIANT value; /* name must be valid for detached attributes */ - WCHAR *name; + BSTR name; HTMLDocumentNode *doc; HTMLElement *elem; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8251
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlelem.c | 43 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c index fcc6394c0d2..6b48ecec5e2 100644 --- a/dlls/mshtml/htmlelem.c +++ b/dlls/mshtml/htmlelem.c @@ -525,6 +525,24 @@ HRESULT create_element(HTMLDocumentNode *doc, const WCHAR *tag, HTMLElement **re return hres; } +static HTMLDOMAttribute *find_attr_in_list(HTMLAttributeCollection *attrs, DISPID dispid, LONG *pos) +{ + HTMLDOMAttribute *iter, *ret = NULL; + struct list *list = &attrs->attrs; + unsigned i = 0; + + LIST_FOR_EACH_ENTRY(iter, list, HTMLDOMAttribute, entry) { + if(iter->dispid == dispid) { + ret = iter; + break; + } + i++; + } + if(pos) + *pos = i; + return ret; +} + typedef struct { DispatchEx dispex; IHTMLRect IHTMLRect_iface; @@ -4390,7 +4408,7 @@ static HRESULT WINAPI HTMLElement4_setAttributeNode(IHTMLElement4 *iface, IHTMLD IHTMLDOMAttribute **ppretAttribute) { HTMLElement *This = impl_from_IHTMLElement4(iface); - HTMLDOMAttribute *attr, *iter, *replace = NULL; + HTMLDOMAttribute *attr, *replace; HTMLAttributeCollection *attrs; DISPID dispid; HRESULT hres; @@ -4414,13 +4432,7 @@ static HRESULT WINAPI HTMLElement4_setAttributeNode(IHTMLElement4 *iface, IHTMLD if(FAILED(hres)) return hres; - LIST_FOR_EACH_ENTRY(iter, &attrs->attrs, HTMLDOMAttribute, entry) { - if(iter->dispid == dispid) { - replace = iter; - break; - } - } - + replace = find_attr_in_list(attrs, dispid, NULL); if(replace) { hres = get_elem_attr_value_by_dispid(This, dispid, &replace->value); if(FAILED(hres)) { @@ -7732,28 +7744,15 @@ static inline HRESULT get_attr_dispid_by_name(HTMLAttributeCollection *This, con static inline HRESULT get_domattr(HTMLAttributeCollection *This, DISPID id, LONG *list_pos, HTMLDOMAttribute **attr) { - HTMLDOMAttribute *iter; - LONG pos = 0; HRESULT hres; - *attr = NULL; - LIST_FOR_EACH_ENTRY(iter, &This->attrs, HTMLDOMAttribute, entry) { - if(iter->dispid == id) { - *attr = iter; - break; - } - pos++; - } - - if(!*attr) { + if(!(*attr = find_attr_in_list(This, id, list_pos))) { hres = HTMLDOMAttribute_Create(NULL, This->elem, id, This->elem->node.doc, attr); if(FAILED(hres)) return hres; } IHTMLDOMAttribute_AddRef(&(*attr)->IHTMLDOMAttribute_iface); - if(list_pos) - *list_pos = pos; return S_OK; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8251
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlelem.c | 25 +++++++++ dlls/mshtml/tests/dom.c | 111 +++++++++++++++++++++++++++++++++++----- 2 files changed, 124 insertions(+), 12 deletions(-) diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c index 6b48ecec5e2..9840f8b0ee8 100644 --- a/dlls/mshtml/htmlelem.c +++ b/dlls/mshtml/htmlelem.c @@ -1331,6 +1331,9 @@ static HRESULT WINAPI HTMLElement_removeAttribute(IHTMLElement *iface, BSTR strA TRACE("(%p)->(%s %lx %p)\n", This, debugstr_w(strAttributeName), lFlags, pfSuccess); if(compat_mode < COMPAT_MODE_IE9 || !This->dom_element) { + HTMLAttributeCollection *attrs; + HTMLDOMAttribute *attr; + hres = dispex_get_id(&This->node.event_target.dispex, translate_attr_name(strAttributeName, compat_mode), lFlags & ATTRFLAG_CASESENSITIVE ? fdexNameCaseSensitive : fdexNameCaseInsensitive, &id); if(hres == DISP_E_UNKNOWNNAME) { @@ -1340,6 +1343,26 @@ static HRESULT WINAPI HTMLElement_removeAttribute(IHTMLElement *iface, BSTR strA if(FAILED(hres)) return hres; + hres = HTMLElement_get_attr_col(&This->node, &attrs); + if(FAILED(hres)) + return hres; + attr = find_attr_in_list(attrs, id, NULL); + IHTMLAttributeCollection_Release(&attrs->IHTMLAttributeCollection_iface); + + if(attr) { + hres = get_elem_attr_value_by_dispid(This, id, &attr->value); + if(FAILED(hres)) + return hres; + if(!attr->name) { + hres = dispex_prop_name(&This->node.event_target.dispex, id, &attr->name); + if(FAILED(hres)) + return hres; + } + attr->elem = NULL; + list_remove(&attr->entry); + IHTMLDOMNode_Release(&This->node.IHTMLDOMNode_iface); + } + if(id == DISPID_IHTMLELEMENT_STYLE) { IHTMLStyle *style; @@ -8203,6 +8226,8 @@ static HRESULT HTMLAttributeCollection_get_dispid(DispatchEx *dispex, const WCHA return hres; IHTMLDOMAttribute_Release(&attr->IHTMLDOMAttribute_iface); + /* Even though this breaks DISPID rules where the same name must return the same DISPID, because the pos can change + * as attributes are removed (and re-added), it's how native works (see test_attr_collection_disp in tests). */ *dispid = MSHTML_DISPID_CUSTOM_MIN+pos; return S_OK; } diff --git a/dlls/mshtml/tests/dom.c b/dlls/mshtml/tests/dom.c index 9b52f7290c7..e29b75562a8 100644 --- a/dlls/mshtml/tests/dom.c +++ b/dlls/mshtml/tests/dom.c @@ -3745,39 +3745,42 @@ static LONG test_attr_collection_attr(IDispatch *attr, LONG i) return ret; } -static void test_attr_collection_disp(IDispatch *disp) +static void test_attr_collection_disp(IDispatch *disp, LONG len, IHTMLElement *elem) { + DISPID dispid, dispid2, dispid_attr1, dispid_attr2, dispid_attr3; IDispatchEx *dispex; IHTMLDOMAttribute *attr; DISPPARAMS dp = {NULL, NULL, 0, 0}; + VARIANT_BOOL vbool; + WCHAR buf[11]; VARIANT var; EXCEPINFO ei; - DISPID id; - BSTR bstr; HRESULT hres; + BSTR bstr; hres = IDispatch_QueryInterface(disp, &IID_IDispatchEx, (void**)&dispex); ok(hres == S_OK, "QueryInterface failed: %08lx\n", hres); - bstr = SysAllocString(L"0"); - hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &id); + swprintf(buf, ARRAY_SIZE(buf), L"%lu", len - 2); + bstr = SysAllocString(buf); + hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &dispid); ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); SysFreeString(bstr); VariantInit(&var); - hres = IDispatchEx_InvokeEx(dispex, id, LOCALE_NEUTRAL, INVOKE_PROPERTYGET, &dp, &var, &ei, NULL); + hres = IDispatchEx_InvokeEx(dispex, dispid, LOCALE_NEUTRAL, INVOKE_PROPERTYGET, &dp, &var, &ei, NULL); ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); ok(V_VT(&var) == VT_DISPATCH, "V_VT(var)=%d\n", V_VT(&var)); ok(V_DISPATCH(&var) != NULL, "V_DISPATCH(var) == NULL\n"); VariantClear(&var); bstr = SysAllocString(L"attr1"); - hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &id); + hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &dispid_attr1); ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); SysFreeString(bstr); VariantInit(&var); - hres = IDispatchEx_InvokeEx(dispex, id, LOCALE_NEUTRAL, INVOKE_PROPERTYGET, &dp, &var, &ei, NULL); + hres = IDispatchEx_InvokeEx(dispex, dispid_attr1, LOCALE_NEUTRAL, INVOKE_PROPERTYGET, &dp, &var, &ei, NULL); ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); ok(V_VT(&var) == VT_DISPATCH, "V_VT(var)=%d\n", V_VT(&var)); ok(V_DISPATCH(&var) != NULL, "V_DISPATCH(var) == NULL\n"); @@ -3789,6 +3792,72 @@ static void test_attr_collection_disp(IDispatch *disp) IHTMLDOMAttribute_Release(attr); VariantClear(&var); + bstr = SysAllocString(L"attr2"); + hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &dispid_attr2); + ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + SysFreeString(bstr); + + bstr = SysAllocString(L"attr3"); + hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &dispid_attr3); + ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + SysFreeString(bstr); + + ok(dispid == dispid_attr1 || dispid == dispid_attr2 || dispid == dispid_attr3, + "indexed dispid isn't one of the attr dispids\n"); + + bstr = SysAllocString(L"attr1"); + hres = IHTMLElement_removeAttribute(elem, bstr, 0, &vbool); + ok(hres == S_OK, "removeAttribute failed: %08lx\n", hres); + ok(vbool == VARIANT_TRUE, "removeAttribute returned %x\n", vbool); + SysFreeString(bstr); + + hres = IDispatchEx_GetMemberName(dispex, dispid_attr1, &bstr); + ok(hres == S_OK, "GetMemberName failed: %08lx\n", hres); + ok(wcscmp(bstr, L"attr1"), "GetMemberName still returned attr1 after removal\n"); + SysFreeString(bstr); + + bstr = SysAllocString(buf); + hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &dispid2); + ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + ok(dispid, "indexed dispid is not the same after removal\n"); + SysFreeString(bstr); + + bstr = SysAllocString(L"attr2"); + hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &dispid); + ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + ok(dispid == dispid_attr2, "dispid != dispid_attr2\n"); + SysFreeString(bstr); + + bstr = SysAllocString(L"attr1"); + hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &dispid); + ok(hres == DISP_E_UNKNOWNNAME, "GetDispID returned: %08lx\n", hres); + SysFreeString(bstr); + + bstr = SysAllocString(L"added_attr"); + V_VT(&var) = VT_BSTR; + V_BSTR(&var) = SysAllocString(L"test"); + hres = IHTMLElement_setAttribute(elem, bstr, var, 0); + ok(hres == S_OK, "setAttribute failed: %08lx\n", hres); + + hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &dispid); + ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + ok(dispid != dispid_attr1, "added attr dispid == dispid_attr1\n"); + ok(dispid != dispid_attr2, "added attr dispid == dispid_attr2\n"); + ok(dispid != dispid_attr3, "added attr dispid == dispid_attr3\n"); + SysFreeString(bstr); + + bstr = SysAllocString(L"attr1"); + hres = IHTMLElement_setAttribute(elem, bstr, var, 0); + ok(hres == S_OK, "setAttribute failed: %08lx\n", hres); + VariantClear(&var); + + hres = IDispatchEx_GetDispID(dispex, bstr, fdexNameCaseSensitive, &dispid); + ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + ok(dispid != dispid_attr1, "attr1 after re-added dispid == dispid_attr1\n"); + ok(dispid != dispid_attr2, "attr1 after re-added dispid == dispid_attr2\n"); + ok(dispid != dispid_attr3, "attr1 after re-added dispid == dispid_attr3\n"); + SysFreeString(bstr); + IDispatchEx_Release(dispex); } @@ -3886,7 +3955,7 @@ static void test_attr_collection(IHTMLElement *elem) ok(hres == E_INVALIDARG, "item failed: %08lx\n", hres); VariantClear(&id); - test_attr_collection_disp(disp); + test_attr_collection_disp(disp, len, elem); IDispatch_Release(disp); IHTMLAttributeCollection_Release(attr_col); @@ -10224,8 +10293,10 @@ static void test_attr(IHTMLDocument2 *doc, IHTMLElement *elem) { IHTMLDOMAttribute *attr, *attr2, *attr3; IHTMLElement4 *elem4; - VARIANT v; + VARIANT_BOOL vbool; HRESULT hres; + VARIANT v; + BSTR bstr; get_elem_attr_node((IUnknown*)elem, L"noattr", FALSE); @@ -10311,6 +10382,24 @@ static void test_attr(IHTMLDocument2 *doc, IHTMLElement *elem) test_attr_specified(attr, VARIANT_FALSE); test_attr_expando(attr, VARIANT_FALSE); test_attr_node(attr, doc); + + attr = get_elem_attr_node((IUnknown*)elem, L"emptyattr", TRUE); + test_attr_specified(attr, VARIANT_TRUE); + test_attr_expando(attr, VARIANT_TRUE); + + bstr = SysAllocString(L"emptyattr"); + hres = IHTMLElement_removeAttribute(elem, bstr, 0, &vbool); + ok(hres == S_OK, "removeAttribute failed: %08lx\n", hres); + ok(vbool == VARIANT_TRUE, "removeAttribute returned %x\n", vbool); + test_attr_expando(attr, VARIANT_FALSE); + SysFreeString(bstr); + + elem4 = get_elem4_iface((IUnknown*)elem); + hres = IHTMLElement4_setAttributeNode(elem4, attr, &attr2); + ok(hres == S_OK, "setAttributeNode failed: %08lx\n", hres); + ok(!attr2, "attr2 != NULL\n"); + test_attr_specified(attr, VARIANT_TRUE); + test_attr_expando(attr, VARIANT_TRUE); IHTMLDOMAttribute_Release(attr); /* Test created, detached attribute. */ @@ -10343,8 +10432,6 @@ static void test_attr(IHTMLDocument2 *doc, IHTMLElement *elem) SysFreeString(V_BSTR(&v)); test_attr_value(attr, L"testing"); - elem4 = get_elem4_iface((IUnknown*)elem); - hres = IHTMLElement4_setAttributeNode(elem4, attr, &attr2); ok(hres == S_OK, "setAttributeNode failed: %08lx\n", hres); ok(!attr2, "attr2 != NULL\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8251
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlattr.c | 4 ++-- dlls/mshtml/tests/dom.c | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/mshtml/htmlattr.c b/dlls/mshtml/htmlattr.c index a83298a7ea1..f98356e19db 100644 --- a/dlls/mshtml/htmlattr.c +++ b/dlls/mshtml/htmlattr.c @@ -94,8 +94,8 @@ static HRESULT WINAPI HTMLDOMAttribute_get_specified(IHTMLDOMAttribute *iface, V TRACE("(%p)->(%p)\n", This, p); if(!This->elem || !This->elem->dom_element) { - FIXME("NULL This->elem\n"); - return E_UNEXPECTED; + *p = VARIANT_FALSE; + return S_OK; } if(get_dispid_type(This->dispid) != DISPEXPROP_BUILTIN) { diff --git a/dlls/mshtml/tests/dom.c b/dlls/mshtml/tests/dom.c index e29b75562a8..db24a1610b3 100644 --- a/dlls/mshtml/tests/dom.c +++ b/dlls/mshtml/tests/dom.c @@ -10391,6 +10391,7 @@ static void test_attr(IHTMLDocument2 *doc, IHTMLElement *elem) hres = IHTMLElement_removeAttribute(elem, bstr, 0, &vbool); ok(hres == S_OK, "removeAttribute failed: %08lx\n", hres); ok(vbool == VARIANT_TRUE, "removeAttribute returned %x\n", vbool); + test_attr_specified(attr, VARIANT_FALSE); test_attr_expando(attr, VARIANT_FALSE); SysFreeString(bstr); @@ -10410,6 +10411,7 @@ static void test_attr(IHTMLDocument2 *doc, IHTMLElement *elem) test_no_iface((IUnknown*)attr, &IID_IHTMLDOMNode); test_attr_node_name(attr, L"Test"); + test_attr_specified(attr, VARIANT_FALSE); test_attr_expando(attr, VARIANT_FALSE); get_attr_node_value(attr, &v, VT_EMPTY); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8251
Jacek Caban (@jacek) commented about dlls/mshtml/htmlelem.c:
+ return hres; + attr = find_attr_in_list(attrs, id, NULL); + IHTMLAttributeCollection_Release(&attrs->IHTMLAttributeCollection_iface); + + if(attr) { + hres = get_elem_attr_value_by_dispid(This, id, &attr->value); + if(FAILED(hres)) + return hres; + if(!attr->name) { + hres = dispex_prop_name(&This->node.event_target.dispex, id, &attr->name); + if(FAILED(hres)) + return hres; + } + attr->elem = NULL; + list_remove(&attr->entry); + IHTMLDOMNode_Release(&This->node.IHTMLDOMNode_iface); I think it would be better to use `attr->elem` for the release. I know that it's the same in this case, but that would make it self-explanatory.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8251#note_110050
Jacek Caban (@jacek) commented about dlls/mshtml/htmlelem.c:
return hres; }
+static HTMLDOMAttribute *find_attr_in_list(HTMLAttributeCollection *attrs, DISPID dispid, LONG *pos) +{ + HTMLDOMAttribute *iter, *ret = NULL; + struct list *list = &attrs->attrs; + unsigned i = 0; + + LIST_FOR_EACH_ENTRY(iter, list, HTMLDOMAttribute, entry) { + if(iter->dispid == dispid) { + ret = iter; + break;
There is no need for `ret` variable, you may just return here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8251#note_110049
On Tue Jul 15 18:02:25 2025 +0000, Gabriel Ivăncescu wrote:
Alright, I actually split it off this MR completely and will be split in next couple MRs. FWIW, I don't think it's necessary to split this into multiple MRs. Once the core logic is in place, it's straightforward enough to include multiple interfaces per commit. My main concern is keeping the key commit that introduces the new logic as small and focused as possible.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8251#note_110052
On Wed Jul 16 09:12:00 2025 +0000, Jacek Caban wrote:
There is no need for `ret` variable, you may just return here. What about filling `pos`?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8251#note_110082
On Wed Jul 16 09:15:11 2025 +0000, Jacek Caban wrote:
FWIW, I don't think it's necessary to split this into multiple MRs. Once the core logic is in place, it's straightforward enough to include multiple interfaces per commit. My main concern is keeping the key commit that introduces the new logic as small and focused as possible. Do you mean the next MR, or to include it in this one? I'd rather keep it for next MR since this one is a bit longer now and full of some misc stuff (I also added a fix for the name in patch 2).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8251#note_110083
On Wed Jul 16 15:48:03 2025 +0000, Gabriel Ivăncescu wrote:
Do you mean the next MR, or to include it in this one? I'd rather keep it for next MR since this one is a bit longer now and full of some misc stuff (I also added a fix for the name in patch 2). Next MR, I meant that there is no need for multiple MRs for that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8251#note_110138
On Wed Jul 16 15:46:59 2025 +0000, Gabriel Ivăncescu wrote:
What about filling `pos`? You may set it here, there is no need to set it when attr is not found.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8251#note_110139
On Wed Jul 16 22:41:47 2025 +0000, Jacek Caban wrote:
You may set it here, there is no need to set it when attr is not found. Are you sure? `HTMLAttributeCollection_get_dispid` (the only one that uses pos) expects it to be filled to obtain the DISPID, even if the attr doesn't exist, because it creates it on-demand, and in this case pos is set to it (it's set before the attr is created to the length of the list basically). Or am I missing something?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8251#note_110196
On Thu Jul 17 15:39:30 2025 +0000, Gabriel Ivăncescu wrote:
Are you sure? `HTMLAttributeCollection_get_dispid` (the only one that uses pos) expects it to be filled to obtain the DISPID, even if the attr doesn't exist, because it creates it on-demand, and in this case pos is set to it (it's set before the attr is created to the length of the list basically). Or am I missing something? I see, that's fine then.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8251#note_110204
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)