[PATCH v3 0/3] MR1031: mshtml: Document element props improvements.
-- v3: mshtml: Don't create dynamic prop before checking if elem prop even exists. mshtml: Allow accessing some document elements as props via id. mshtml: Expose props via element name only for specific element types. 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 | 31 +++++++++++++-------- dlls/mshtml/tests/documentmode.js | 46 ++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 13 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index d86ed07e033..0e35830552c 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -4820,23 +4820,30 @@ static inline HTMLDocument *impl_from_IDispatchEx(IDispatchEx *iface) static HRESULT has_elem_name(nsIDOMHTMLDocument *nsdoc, const WCHAR *name) { - nsIDOMNodeList *node_list; - nsAString name_str; + static const WCHAR fmt[] = L":-moz-any(applet,embed,form,iframe,img,object)[name=%s]"; + WCHAR buf[128], *selector = buf; + nsAString selector_str; + nsIDOMElement *nselem; nsresult nsres; - UINT32 len; + size_t len; - nsAString_InitDepend(&name_str, name); - nsres = nsIDOMHTMLDocument_GetElementsByName(nsdoc, &name_str, &node_list); - nsAString_Finish(&name_str); - if(NS_FAILED(nsres)) - return map_nsresult(nsres); + len = wcslen(name) + ARRAY_SIZE(fmt) - 2 /* %s */; + if(len > ARRAY_SIZE(buf) && !(selector = heap_alloc(len * sizeof(WCHAR)))) + return E_OUTOFMEMORY; + swprintf(selector, len, fmt, name); - nsres = nsIDOMNodeList_GetLength(node_list, &len); - nsIDOMNodeList_Release(node_list); + nsAString_InitDepend(&selector_str, selector); + nsres = nsIDOMHTMLDocument_QuerySelector(nsdoc, &selector_str, &nselem); + nsAString_Finish(&selector_str); + if(selector != buf) + heap_free(selector); if(NS_FAILED(nsres)) return map_nsresult(nsres); - return len ? S_OK : DISP_E_UNKNOWNNAME; + if(!nselem) + return DISP_E_UNKNOWNNAME; + nsIDOMElement_Release(nselem); + return S_OK; } static HRESULT dispid_from_elem_name(HTMLDocumentNode *This, const WCHAR *name, DISPID *dispid) @@ -5918,7 +5925,7 @@ static HRESULT HTMLDocumentNode_next_dispid(DispatchEx *dispex, DISPID id, DISPI } /* Populate possibly missing DISPIDs */ - nsAString_InitDepend(&nsstr, L"[name]"); + nsAString_InitDepend(&nsstr, L":-moz-any(applet,embed,form,iframe,img,object)[name]"); nsres = nsIDOMHTMLDocument_QuerySelectorAll(This->nsdoc, &nsstr, &node_list); nsAString_Finish(&nsstr); if(NS_FAILED(nsres)) diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index ad258b3df2e..9fb17422613 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -669,7 +669,7 @@ sync_test("for..in", function() { sync_test("elem_by_id", function() { document.body.innerHTML = '<form id="testid" name="testname"></form>'; - var found; + var found, i; var id_elem = document.getElementById("testid"); ok(id_elem.tagName === "FORM", "id_elem.tagName = " + id_elem.tagName); @@ -704,6 +704,50 @@ sync_test("elem_by_id", function() { found = true; } ok(found, "testname was not enumerated in document"); + + // these tags expose name as props, and id only if they have a name + var tags = [ "embed", "form", "iframe", "img" ]; + for(i = 0; i < tags.length; i++) { + document.body.innerHTML = '<' + tags[i] + ' id="testid" name="testname"></' + tags[i] + '><' + tags[i] + ' id="foobar"></' + tags[i] + '>'; + ok("testname" in document, tags[i] + " did not expose testname"); + todo_wine. + ok("testid" in document, tags[i] + " did not expose testid"); + ok(!("foobar" in document), tags[i] + " exposed foobar"); + } + + // these tags always expose their id as well as name (we don't test applet because it makes Windows pop up a dialog box) + var tags = [ "object" ]; + for(i = 0; i < tags.length; i++) { + document.body.innerHTML = '<' + tags[i] + ' id="testid" name="testname"></' + tags[i] + '><' + tags[i] + ' id="foobar"></' + tags[i] + '>'; + ok("testname" in document, tags[i] + " did not expose testname"); + todo_wine. + ok("testid" in document, tags[i] + " did not expose testid"); + todo_wine. + ok("foobar" in document, tags[i] + " did not expose foobar"); + } + + // all other tags don't expose props for either id or name + tags = [ + "abbr", "acronym", "address", "article", "aside", "audio", "b", "base", "basefont", "bdi", "bdo", "big", "blockquote", "body", + "br", "button", "canvas", "caption", "center", "cite", "code", "col", "colgroup", "data", "datalist", "dd", "del", "details", + "dfn", "dialog", "dir", "div", "dl", "dt", "em", "fieldset", "figcaption", "figure", "font", "footer", "frame", "frameset", + "h1", "h2", "h3", "h4", "h5", "h6", "h7", "head", "header", "hr", "html", "i", "input", "ins", "kbd", "label", "legend", "li", + "link", "main", "map", "mark", "meta", "meter", "nav", "noframes", "noscript", "ol", "optgroup", "option", "output", "p", + "param", "picture", "pre", "progress", "q", "rp", "rt", "ruby", "s", "samp", "script", "section", "select", "small", "source", + "span", "strike", "strong", "style", "sub", "summary", "sup", "svg", "table", "tbody", "td", "template", "textarea", "tfoot", + "th", "thead", "time", "title", "tr", "track", "tt", "u", "ul", "var", "video", "wbr", "winetest" + ]; + for(i = 0; i < tags.length; i++) { + // IE7 and below throw "Unknown Runtime Error" for some tags, like frameset + try { + document.body.innerHTML = '<' + tags[i] + ' id="testid" name="testname"></' + tags[i] + '><' + tags[i] + ' id="foobar"></' + tags[i] + '>'; + }catch(e) { + continue; + } + ok(!("testname" in document), tags[i] + " exposed testname"); + ok(!("testid" in document), tags[i] + " exposed testid"); + ok(!("foobar" in document), tags[i] + " exposed foobar"); + } }); sync_test("doc_mode", function() { -- 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 | 58 +++++++++++++++++++++++-------- dlls/mshtml/tests/documentmode.js | 3 -- 2 files changed, 43 insertions(+), 18 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index 0e35830552c..bf19ec335f7 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -4846,6 +4846,41 @@ static HRESULT has_elem_name(nsIDOMHTMLDocument *nsdoc, const WCHAR *name) return S_OK; } +static HRESULT get_elem_by_name_or_id(nsIDOMHTMLDocument *nsdoc, const WCHAR *name, nsIDOMElement **ret) +{ + static const WCHAR fmt[] = L":-moz-any(embed,form,iframe,img):-moz-any([name=%s],[id=%s][name])," + L":-moz-any(applet,object):-moz-any([name=%s],[id=%s])"; + WCHAR buf[384], *selector = buf; + nsAString selector_str; + nsIDOMElement *nselem; + nsresult nsres; + size_t len; + + len = wcslen(name) * 4 + ARRAY_SIZE(fmt) - 8 /* %s */; + if(len > ARRAY_SIZE(buf) && !(selector = heap_alloc(len * sizeof(WCHAR)))) + return E_OUTOFMEMORY; + swprintf(selector, len, fmt, name, name, name, name); + + nsAString_InitDepend(&selector_str, selector); + nsres = nsIDOMHTMLDocument_QuerySelector(nsdoc, &selector_str, &nselem); + nsAString_Finish(&selector_str); + if(selector != buf) + heap_free(selector); + if(NS_FAILED(nsres)) + return map_nsresult(nsres); + + if(ret) { + *ret = nselem; + return S_OK; + } + + if(nselem) { + nsIDOMElement_Release(nselem); + return S_OK; + } + return DISP_E_UNKNOWNNAME; +} + static HRESULT dispid_from_elem_name(HTMLDocumentNode *This, const WCHAR *name, DISPID *dispid) { unsigned i; @@ -4951,7 +4986,7 @@ static HRESULT WINAPI DocDispatchEx_GetDispID(IDispatchEx *iface, BSTR bstrName, return hres; if(This->doc_node->nsdoc) { - hres = has_elem_name(This->doc_node->nsdoc, bstrName); + hres = get_elem_by_name_or_id(This->doc_node->nsdoc, bstrName, NULL); if(SUCCEEDED(hres)) hres = dispid_from_elem_name(This->doc_node, bstrName, pid); } @@ -5859,12 +5894,9 @@ static HRESULT HTMLDocumentNode_invoke(DispatchEx *dispex, DISPID id, LCID lcid, VARIANT *res, EXCEPINFO *ei, IServiceProvider *caller) { HTMLDocumentNode *This = impl_from_DispatchEx(dispex); - nsIDOMNodeList *node_list; - nsAString name_str; - nsIDOMNode *nsnode; + nsIDOMElement *nselem; HTMLDOMNode *node; unsigned i; - nsresult nsres; HRESULT hres; if(flags != DISPATCH_PROPERTYGET && flags != (DISPATCH_METHOD|DISPATCH_PROPERTYGET)) { @@ -5877,18 +5909,14 @@ 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) + hres = get_elem_by_name_or_id(This->nsdoc, This->elem_vars[i], &nselem); + if(FAILED(hres)) + return hres; + if(!nselem) return DISP_E_MEMBERNOTFOUND; - hres = get_node(nsnode, TRUE, &node); + hres = get_node((nsIDOMNode*)nselem, TRUE, &node); + nsIDOMElement_Release(nselem); if(FAILED(hres)) return hres; diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 9fb17422613..c21899ebaa6 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -710,7 +710,6 @@ sync_test("elem_by_id", function() { for(i = 0; i < tags.length; i++) { document.body.innerHTML = '<' + tags[i] + ' id="testid" name="testname"></' + tags[i] + '><' + tags[i] + ' id="foobar"></' + tags[i] + '>'; ok("testname" in document, tags[i] + " did not expose testname"); - todo_wine. ok("testid" in document, tags[i] + " did not expose testid"); ok(!("foobar" in document), tags[i] + " exposed foobar"); } @@ -720,9 +719,7 @@ sync_test("elem_by_id", function() { for(i = 0; i < tags.length; i++) { document.body.innerHTML = '<' + tags[i] + ' id="testid" name="testname"></' + tags[i] + '><' + tags[i] + ' id="foobar"></' + tags[i] + '>'; ok("testname" in document, tags[i] + " did not expose testname"); - todo_wine. ok("testid" in document, tags[i] + " did not expose testid"); - todo_wine. ok("foobar" in document, tags[i] + " did not expose foobar"); } -- 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 | 49 +++++++++++++++++++++++++++++-- 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index bf19ec335f7..d7ca1806507 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -4981,7 +4981,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; @@ -4990,6 +4990,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; } @@ -5899,10 +5902,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 c21899ebaa6..7960b674196 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>'; - var found, i; + var v = document.documentMode, found, i; 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"); @@ -705,6 +705,51 @@ sync_test("elem_by_id", function() { } ok(found, "testname was not enumerated in document"); + try { + document.testname(); + ok(false, "document.testname() did not throw exception"); + }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"); + delete document.testid; + delete document.testname; + }catch(e) { + todo_wine_if(v >= 9). + ok(v < 9 && e.number === 0xa01b6 - 0x80000000, "Setting document.testid threw = " + e.number); + } + // these tags expose name as props, and id only if they have a name var tags = [ "embed", "form", "iframe", "img" ]; for(i = 0; i < tags.length; i++) { -- 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=124927 Your paranoid android. === w10pro64_ja (64 bit report) === mshtml: htmldoc.c:350: Test failed: expected Exec_SETTITLE htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE === debian11 (32 bit report) === mmdevapi: render.c:1116: Test marked flaky: Position 54240 too far after playing 100ms === debian11 (build log) === Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24702. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24702. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24702.
Jacek Caban (@jacek) commented about dlls/mshtml/tests/documentmode.js:
+ "abbr", "acronym", "address", "article", "aside", "audio", "b", "base", "basefont", "bdi", "bdo", "big", "blockquote", "body", + "br", "button", "canvas", "caption", "center", "cite", "code", "col", "colgroup", "data", "datalist", "dd", "del", "details", + "dfn", "dialog", "dir", "div", "dl", "dt", "em", "fieldset", "figcaption", "figure", "font", "footer", "frame", "frameset", + "h1", "h2", "h3", "h4", "h5", "h6", "h7", "head", "header", "hr", "html", "i", "input", "ins", "kbd", "label", "legend", "li", + "link", "main", "map", "mark", "meta", "meter", "nav", "noframes", "noscript", "ol", "optgroup", "option", "output", "p", + "param", "picture", "pre", "progress", "q", "rp", "rt", "ruby", "s", "samp", "script", "section", "select", "small", "source", + "span", "strike", "strong", "style", "sub", "summary", "sup", "svg", "table", "tbody", "td", "template", "textarea", "tfoot", + "th", "thead", "time", "title", "tr", "track", "tt", "u", "ul", "var", "video", "wbr", "winetest" + ]; + for(i = 0; i < tags.length; i++) { + // IE7 and below throw "Unknown Runtime Error" for some tags, like frameset + try { + document.body.innerHTML = '<' + tags[i] + ' id="testid" name="testname"></' + tags[i] + '><' + tags[i] + ' id="foobar"></' + tags[i] + '>'; + }catch(e) { + continue; + } I think it would be better to simply drop problematic tags. It's not really interesting to test that many tags anyway, please just pick a few.
BTW, you could also consider using something like `for (tag in tags)` loop. Other than that, patches look good now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1031#note_10549
participants (3)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)
-
Marvin