[PATCH v2 0/2] MR1031: mshtml: Document element props improvements.
-- v2: mshtml: Don't create dynamic prop before checking if elem prop even exists. mshtml: Allow accessing document elements as props via id if they have name. https://gitlab.winehq.org/wine/wine/-/merge_requests/1031
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmldoc.c | 55 +++++++++++++++++++++---------- dlls/mshtml/tests/documentmode.js | 13 ++++++-- 2 files changed, 47 insertions(+), 21 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index d86ed07e033..ebc8aadc930 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -4839,6 +4839,28 @@ static HRESULT has_elem_name(nsIDOMHTMLDocument *nsdoc, const WCHAR *name) return len ? S_OK : DISP_E_UNKNOWNNAME; } +static HRESULT has_elem_id(nsIDOMHTMLDocument *nsdoc, const WCHAR *id) +{ + nsIDOMElement *nselem; + cpp_bool has_name; + nsAString nsstr; + nsresult nsres; + + nsAString_InitDepend(&nsstr, id); + nsres = nsIDOMHTMLDocument_GetElementById(nsdoc, &nsstr, &nselem); + nsAString_Finish(&nsstr); + if(NS_FAILED(nsres)) + return map_nsresult(nsres); + if(!nselem) + return DISP_E_UNKNOWNNAME; + + nsAString_InitDepend(&nsstr, L"name"); + nsIDOMElement_HasAttribute(nselem, &nsstr, &has_name); + nsIDOMElement_Release(nselem); + nsAString_Finish(&nsstr); + return has_name ? S_OK : DISP_E_UNKNOWNNAME; +} + static HRESULT dispid_from_elem_name(HTMLDocumentNode *This, const WCHAR *name, DISPID *dispid) { unsigned i; @@ -4945,6 +4967,8 @@ static HRESULT WINAPI DocDispatchEx_GetDispID(IDispatchEx *iface, BSTR bstrName, if(This->doc_node->nsdoc) { hres = has_elem_name(This->doc_node->nsdoc, bstrName); + if(hres == DISP_E_UNKNOWNNAME) + hres = has_elem_id(This->doc_node->nsdoc, bstrName); if(SUCCEEDED(hres)) hres = dispid_from_elem_name(This->doc_node, bstrName, pid); } @@ -5851,13 +5875,11 @@ static HRESULT HTMLDocumentNode_get_name(DispatchEx *dispex, DISPID id, BSTR *na static HRESULT HTMLDocumentNode_invoke(DispatchEx *dispex, DISPID id, LCID lcid, WORD flags, DISPPARAMS *params, VARIANT *res, EXCEPINFO *ei, IServiceProvider *caller) { + static WCHAR nameW[] = L"name"; HTMLDocumentNode *This = impl_from_DispatchEx(dispex); - nsIDOMNodeList *node_list; - nsAString name_str; - nsIDOMNode *nsnode; - HTMLDOMNode *node; + HTMLElement *elem; + VARIANT_BOOL b; unsigned i; - nsresult nsres; HRESULT hres; if(flags != DISPATCH_PROPERTYGET && flags != (DISPATCH_METHOD|DISPATCH_PROPERTYGET)) { @@ -5870,23 +5892,20 @@ static HRESULT HTMLDocumentNode_invoke(DispatchEx *dispex, DISPID id, LCID lcid, if(!This->nsdoc || i >= This->elem_vars_cnt) return DISP_E_MEMBERNOTFOUND; - nsAString_InitDepend(&name_str, This->elem_vars[i]); - nsres = nsIDOMHTMLDocument_GetElementsByName(This->nsdoc, &name_str, &node_list); - nsAString_Finish(&name_str); - if(NS_FAILED(nsres)) - return E_FAIL; - - nsres = nsIDOMNodeList_Item(node_list, 0, &nsnode); - nsIDOMNodeList_Release(node_list); - if(NS_FAILED(nsres) || !nsnode) - return DISP_E_MEMBERNOTFOUND; - - hres = get_node(nsnode, TRUE, &node); + hres = get_doc_elem_by_id(This, This->elem_vars[i], &elem); if(FAILED(hres)) return hres; + if(!elem) + return DISP_E_MEMBERNOTFOUND; + + hres = IHTMLElement6_hasAttribute(&elem->IHTMLElement6_iface, nameW, &b); + if(FAILED(hres) || !b) { + IHTMLElement6_Release(&elem->IHTMLElement6_iface); + return FAILED(hres) ? hres : DISP_E_MEMBERNOTFOUND; + } V_VT(res) = VT_DISPATCH; - V_DISPATCH(res) = (IDispatch*)&node->IHTMLDOMNode_iface; + V_DISPATCH(res) = (IDispatch*)&elem->node.IHTMLDOMNode_iface; return S_OK; } diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index ad258b3df2e..32f69789e55 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -668,7 +668,7 @@ sync_test("for..in", function() { }); sync_test("elem_by_id", function() { - document.body.innerHTML = '<form id="testid" name="testname"></form>'; + document.body.innerHTML = '<form id="testid" name="testname"></form><div id="foobar"></div>'; var found; var id_elem = document.getElementById("testid"); @@ -682,16 +682,23 @@ sync_test("elem_by_id", function() { id_elem = window.testid; ok(id_elem.tagName === "FORM", "window.testid = " + id_elem); + name_elem = window.testname; + ok(name_elem.tagName === "FORM", "window.testname = " + name_elem); + id_elem = window.foobar; + ok(id_elem.tagName === "DIV", "window.foobar = " + id_elem); + id_elem = document.testid; + ok(id_elem.tagName === "FORM", "document.testid = " + id_elem); name_elem = document.testname; ok(name_elem.tagName === "FORM", "document.testname = " + name_elem); + ok(!("foobar" in document), "foobar in document"); for(id_elem in window) - ok(id_elem !== "testid" && id_elem != "testname", id_elem + " was enumerated in window"); + ok(id_elem !== "testid" && id_elem != "testname" && id_elem != "foobar", id_elem + " was enumerated in window"); window.testid = 137; found = false; for(id_elem in window) { - ok(id_elem != "testname", id_elem + " was enumerated in window after set to 137"); + ok(id_elem != "testname" && id_elem != "foobar", id_elem + " was enumerated in window after set to 137"); if(id_elem === "testid") found = true; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1031
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmldoc.c | 11 ++++---- dlls/mshtml/mshtml_private.h | 1 + dlls/mshtml/tests/documentmode.js | 46 +++++++++++++++++++++++++++++-- 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index ebc8aadc930..a18683e67b1 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -4961,7 +4961,7 @@ static HRESULT WINAPI DocDispatchEx_GetDispID(IDispatchEx *iface, BSTR bstrName, HTMLDocument *This = impl_from_IDispatchEx(iface); HRESULT hres; - hres = IDispatchEx_GetDispID(This->dispex, bstrName, grfdex, pid); + hres = IDispatchEx_GetDispID(This->dispex, bstrName, grfdex & ~fdexNameEnsure, pid); if(hres != DISP_E_UNKNOWNNAME) return hres; @@ -4972,6 +4972,9 @@ static HRESULT WINAPI DocDispatchEx_GetDispID(IDispatchEx *iface, BSTR bstrName, if(SUCCEEDED(hres)) hres = dispid_from_elem_name(This->doc_node, bstrName, pid); } + + if(hres == DISP_E_UNKNOWNNAME && (grfdex & fdexNameEnsure)) + hres = IDispatchEx_GetDispID(This->dispex, bstrName, grfdex, pid); return hres; } @@ -5882,10 +5885,8 @@ static HRESULT HTMLDocumentNode_invoke(DispatchEx *dispex, DISPID id, LCID lcid, unsigned i; HRESULT hres; - if(flags != DISPATCH_PROPERTYGET && flags != (DISPATCH_METHOD|DISPATCH_PROPERTYGET)) { - FIXME("unsupported flags %x\n", flags); - return E_NOTIMPL; - } + if(flags != DISPATCH_PROPERTYGET && flags != (DISPATCH_METHOD|DISPATCH_PROPERTYGET)) + return MSHTML_E_INVALID_PROPERTY; i = id - MSHTML_DISPID_CUSTOM_MIN; diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index 4ce90d3f17e..7ac4cea2e2b 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -72,6 +72,7 @@ #define NSAPI WINAPI +#define MSHTML_E_INVALID_PROPERTY 0x800a01b6 #define MSHTML_E_INVALID_ACTION 0x800a01bd #define MSHTML_E_NODOC 0x800a025c diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 32f69789e55..2a060cebcea 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -669,13 +669,13 @@ sync_test("for..in", function() { sync_test("elem_by_id", function() { document.body.innerHTML = '<form id="testid" name="testname"></form><div id="foobar"></div>'; - var found; + var v = document.documentMode, found; var id_elem = document.getElementById("testid"); ok(id_elem.tagName === "FORM", "id_elem.tagName = " + id_elem.tagName); var name_elem = document.getElementById("testname"); - if(document.documentMode < 8) + if(v < 8) ok(id_elem === name_elem, "id_elem != id_elem"); else ok(name_elem === null, "name_elem != null"); @@ -711,6 +711,48 @@ sync_test("elem_by_id", function() { found = true; } ok(found, "testname was not enumerated in document"); + + try { + document.testname(); + }catch(e) { + ok(e.number === 0xa01b6 - 0x80000000, "document.testname() threw = " + e.number); + } + + try { + document.testname = "foo"; + ok(v >= 9, "Setting document.testname did not throw exception"); + + id_elem = document.testid; + ok(id_elem.tagName === "FORM", "document.testid after set = " + id_elem); + name_elem = document.testname; + ok(name_elem === "foo", "document.testname after set = " + name_elem); + }catch(e) { + todo_wine_if(v >= 9). + ok(v < 9 && e.number === 0xa01b6 - 0x80000000, "Setting document.testname threw = " + e.number); + } + + try { + document.testid = "bar"; + ok(v >= 9, "Setting document.testid did not throw exception"); + + id_elem = document.testid; + ok(id_elem === "bar", "document.testid after both set = " + id_elem); + name_elem = document.testname; + ok(name_elem === "foo", "document.testname after both set = " + name_elem); + + found = false, name_elem = false; + for(id_elem in document) { + if(id_elem === "testid") + found = true; + if(id_elem === "testname") + name_elem = true; + } + ok(found, "testid was not enumerated in document after both set"); + ok(name_elem, "testname was not enumerated in document after both set"); + }catch(e) { + todo_wine_if(v >= 9). + ok(v < 9 && e.number === 0xa01b6 - 0x80000000, "Setting document.testid threw = " + e.number); + } }); sync_test("doc_mode", function() { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1031
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124893 Your paranoid android. === w10pro64_ar (64 bit report) === mshtml: htmldoc.c:350: Test failed: expected Exec_SETTITLE htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE === debian11 (build log) === Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24690. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24690. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24690.
Jacek Caban (@jacek) commented about dlls/mshtml/htmldoc.c:
+ nsAString nsstr; + nsresult nsres; + + nsAString_InitDepend(&nsstr, id); + nsres = nsIDOMHTMLDocument_GetElementById(nsdoc, &nsstr, &nselem); + nsAString_Finish(&nsstr); + if(NS_FAILED(nsres)) + return map_nsresult(nsres); + if(!nselem) + return DISP_E_UNKNOWNNAME; + + nsAString_InitDepend(&nsstr, L"name"); + nsIDOMElement_HasAttribute(nselem, &nsstr, &has_name); + nsIDOMElement_Release(nselem); + nsAString_Finish(&nsstr); + return has_name ? S_OK : DISP_E_UNKNOWNNAME; This looks suspicious and quick testing doesn't confirm your theory. If I add name attribute to <div> from your test, it still passes on Windows, but not with your patches.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1031#note_10341
Jacek Caban (@jacek) commented about dlls/mshtml/tests/documentmode.js:
found = true; } ok(found, "testname was not enumerated in document"); + + try { + document.testname(); + }catch(e) { + ok(e.number === 0xa01b6 - 0x80000000, "document.testname() threw = " + e.number); + }
It would be better to make sure that the exception is thrown. This test will succeed if no exception is thrown at all (it's similar in other cases). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1031#note_10342
On Tue Oct 11 21:02:34 2022 +0000, Jacek Caban wrote:
It would be better to make sure that the exception is thrown. This test will succeed if no exception is thrown at all (it's similar in other cases). Oops, I usually always included an ok(false, ...), not sure why I forgot here.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1031#note_10407
On Tue Oct 11 21:18:58 2022 +0000, Jacek Caban wrote:
This looks suspicious and quick testing doesn't confirm your theory. If I add name attribute to `<div>` from your test, it still passes on Windows, but not with your patches, see https://testbot.winehq.org/JobDetails.pl?Key=124900 So, this appears it depends on the tag. The patch is correct for `embed`, `form`, `iframe`, and `img` tags, where they need to have `name` to expose their `id` as a prop. `applet` and `object` always expose the `id` as a prop, regardless of if they have a `name` or not.
All the other tags never expose a prop for either id or name, including `div` (which is what I used to test). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1031#note_10408
participants (3)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)
-
Marvin