[PATCH v3 1/3] mshtml: Hold a ref to the location on the window side.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlwindow.c | 5 ++--- dlls/mshtml/tests/dom.c | 4 +--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index 0b25e71..635b046 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -68,9 +68,7 @@ static inline BOOL is_outer_window(HTMLWindow *window) static HRESULT get_location(HTMLInnerWindow *This, HTMLLocation **ret) { - if(This->location) { - IHTMLLocation_AddRef(&This->location->IHTMLLocation_iface); - }else { + if(!This->location) { HRESULT hres; hres = HTMLLocation_Create(This, &This->location); @@ -78,6 +76,7 @@ static HRESULT get_location(HTMLInnerWindow *This, HTMLLocation **ret) return hres; } + IHTMLLocation_AddRef(&This->location->IHTMLLocation_iface); *ret = This->location; return S_OK; } diff --git a/dlls/mshtml/tests/dom.c b/dlls/mshtml/tests/dom.c index 03f5c04..ea5b210 100644 --- a/dlls/mshtml/tests/dom.c +++ b/dlls/mshtml/tests/dom.c @@ -6015,7 +6015,6 @@ static void test_location(IHTMLDocument2 *doc) IHTMLLocation *location, *location2; IHTMLWindow2 *window; BSTR str; - ULONG ref; HRESULT hres; hres = IHTMLDocument2_get_location(doc, &location); @@ -6051,8 +6050,7 @@ static void test_location(IHTMLDocument2 *doc) ok(!lstrcmpW(str, L"about:blank"), "unexpected href %s\n", wine_dbgstr_w(str)); SysFreeString(str); - ref = IHTMLLocation_Release(location); - ok(!ref, "location should be destroyed here\n"); + IHTMLLocation_Release(location); } static void test_plugins_col(IHTMLDocument2 *doc) -- 2.31.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlelem.c | 98 ++++++++++++++++++++++++++++++- dlls/mshtml/tests/documentmode.js | 63 ++++++++++++++++++++ 2 files changed, 160 insertions(+), 1 deletion(-) diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c index de69e2f..503a287 100644 --- a/dlls/mshtml/htmlelem.c +++ b/dlls/mshtml/htmlelem.c @@ -355,6 +355,98 @@ static inline HTMLElement *impl_from_IHTMLElement(IHTMLElement *iface) return CONTAINING_RECORD(iface, HTMLElement, IHTMLElement_iface); } +static HRESULT create_nselem_parse(HTMLDocumentNode *doc, const WCHAR *tag, nsIDOMElement **ret) +{ + WCHAR *p = wcschr(tag + 1, '>'); + nsIDOMHTMLCollection *nscol; + UINT32 i, name_len, length; + nsIDOMHTMLElement *nshtml; + HRESULT hres = E_FAIL; + nsresult nsres; + nsAString str; + + if(!p || p[1] || wcschr(tag + 1, '<')) + return E_FAIL; + if(!doc->nsdoc) { + WARN("NULL nsdoc\n"); + return E_UNEXPECTED; + } + + /* Ignore the > or /> end token */ + length = p - tag - (p[-1] == '/'); + + /* Get the tag name using HTML whitespace rules */ + name_len = length - 1; + for(i = 1; i < length; i++) { + if((tag[i] >= 0x09 && tag[i] <= 0x0d) || tag[i] == ' ') { + name_len = i - 1; + break; + } + } + if(!name_len) + return E_FAIL; + + /* Create a temporary html element and parse it there */ + nsAString_InitDepend(&str, L"HTML"); + nsres = nsIDOMHTMLDocument_CreateElement(doc->nsdoc, &str, (nsIDOMElement**)&nshtml); + nsAString_Finish(&str); + if(NS_FAILED(nsres)) + return map_nsresult(nsres); + + if(name_len == 4 && !wcsnicmp(tag + 1, L"HTML", 4)) { + FIXME("Returning <html> element with no attributes\n"); + *ret = (nsIDOMElement*)nshtml; + return S_OK; + } + + nsAString_InitDepend(&str, tag); + nsres = nsIDOMHTMLElement_SetInnerHTML(nshtml, &str); + nsAString_Finish(&str); + if(NS_FAILED(nsres)) { + hres = map_nsresult(nsres); + goto fail; + } + + /* Get the element and remove it from the temporary */ + if(!(p = heap_alloc((name_len + 1) * sizeof(WCHAR)))) + hres = E_OUTOFMEMORY; + else { + memcpy(p, tag + 1, name_len * sizeof(WCHAR)); + p[name_len] = '\0'; + nsAString_InitDepend(&str, p); + nsres = nsIDOMHTMLElement_GetElementsByTagName(nshtml, &str, &nscol); + nsAString_Finish(&str); + heap_free(p); + if(NS_FAILED(nsres)) + goto fail; + + length = 0; + nsres = nsIDOMHTMLCollection_GetLength(nscol, &length); + if(NS_SUCCEEDED(nsres) && length == 1) { + nsIDOMNode *nsnode, *nsparent, *tmp; + + nsIDOMHTMLCollection_Item(nscol, 0, &nsnode); + nsres = nsIDOMNode_GetParentNode(nsnode, &nsparent); + if(NS_SUCCEEDED(nsres)) { + nsres = nsIDOMNode_RemoveChild(nsparent, nsnode, &tmp); + nsIDOMNode_Release(nsparent); + if(NS_SUCCEEDED(nsres)) { + nsres = nsIDOMNode_QueryInterface(tmp, &IID_nsIDOMElement, (void**)ret); + nsIDOMNode_Release(tmp); + if(NS_SUCCEEDED(nsres)) + hres = S_OK; + } + } + nsIDOMNode_Release(nsnode); + } + nsIDOMHTMLCollection_Release(nscol); + } + +fail: + nsIDOMHTMLElement_Release(nshtml); + return hres; +} + HRESULT create_nselem(HTMLDocumentNode *doc, const WCHAR *tag, nsIDOMElement **ret) { nsAString tag_str; @@ -385,7 +477,11 @@ HRESULT create_element(HTMLDocumentNode *doc, const WCHAR *tag, HTMLElement **re if(!doc->nsdoc) doc = doc->node.doc; - hres = create_nselem(doc, tag, &nselem); + /* IE8 and below allow creating elements with attributes, such as <div class="a"> */ + if(tag[0] == '<' && dispex_compat_mode(&doc->node.event_target.dispex) <= COMPAT_MODE_IE8) + hres = create_nselem_parse(doc, tag, &nselem); + else + hres = create_nselem(doc, tag, &nselem); if(FAILED(hres)) return hres; diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 02a5f5f..69f4796 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -488,6 +488,69 @@ sync_test("style_props", function() { } }); +sync_test("createElement_inline_attr", function() { + var v = document.documentMode, e, s; + + if(v < 9) { + s = document.createElement("<div>").tagName; + ok(s === "DIV", "<div>.tagName returned " + s); + s = document.createElement("<div >").tagName; + ok(s === "DIV", "<div >.tagName returned " + s); + s = document.createElement("<div/>").tagName; + ok(s === "DIV", "<div/>.tagName returned " + s); + e = 0; + try { + document.createElement("<div"); + }catch(ex) { + e = ex.number; + } + ok(e === 0x4005 - 0x80000000, "<div e = " + e); + e = 0; + try { + document.createElement("<div test=1"); + }catch(ex) { + e = ex.number; + } + ok(e === 0x4005 - 0x80000000, "<div test=1 e = " + e); + + var tags = [ "div", "head", "body", "title" ]; + + 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 { + s = ""; + e = 0; + try { + document.createElement("<div>"); + }catch(ex) { + s = ex.toString(); + e = ex.number; + } + todo_wine. + ok(e === undefined, "<div> e = " + e); + todo_wine. + ok(s === "InvalidCharacterError", "<div> s = " + s); + s = ""; + e = 0; + try { + document.createElement("<div test=\"a\">"); + }catch(ex) { + s = ex.toString(); + e = ex.number; + } + todo_wine. + ok(e === undefined, "<div test=\"a\"> e = " + e); + todo_wine. + ok(s === "InvalidCharacterError", "<div test=\"a\"> s = " + s); + } +}); + sync_test("JS objs", function() { var g = window; -- 2.31.1
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=101294 Your paranoid android. === w8adm (32 bit report) === mshtml: events.c:1089: Test failed: unexpected call img_onerror events: Timeout === w10pro64_ar (64 bit report) === mshtml: htmldoc.c:2541: Test failed: unexpected call UpdateUI htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS === w10pro64_ja (64 bit report) === mshtml: htmldoc.c:2541: Test failed: unexpected call UpdateUI htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS === w10pro64_he (testbot log) === WineRunTask.pl:error: The previous 1 run(s) terminated abnormally
On 11/4/21 3:10 PM, Gabriel Ivăncescu wrote:
+ /* Create a temporary html element and parse it there */ + nsAString_InitDepend(&str, L"HTML"); + nsres = nsIDOMHTMLDocument_CreateElement(doc->nsdoc, &str, (nsIDOMElement**)&nshtml); + nsAString_Finish(&str); + if(NS_FAILED(nsres)) + return map_nsresult(nsres); + + if(name_len == 4 && !wcsnicmp(tag + 1, L"HTML", 4)) { + FIXME("Returning <html> element with no attributes\n"); +*ret = (nsIDOMElement*)nshtml; + return S_OK; + }
Did you try using <template> element instead of <html>? I'd expect it do do the right thing, but I didn't try.
+ + nsAString_InitDepend(&str, tag); + nsres = nsIDOMHTMLElement_SetInnerHTML(nshtml, &str); + nsAString_Finish(&str); + if(NS_FAILED(nsres)) { + hres = map_nsresult(nsres); + goto fail; + } + + /* Get the element and remove it from the temporary */ + if(!(p = heap_alloc((name_len + 1) * sizeof(WCHAR)))) + hres = E_OUTOFMEMORY; + else { + memcpy(p, tag + 1, name_len * sizeof(WCHAR)); + p[name_len] = '\0'; + nsAString_InitDepend(&str, p); + nsres = nsIDOMHTMLElement_GetElementsByTagName(nshtml, &str, &nscol); + nsAString_Finish(&str); + heap_free(p); + if(NS_FAILED(nsres)) + goto fail;
That's making things more complicated that they need to be. You could just use firstElementChild or something similar. Thanks, Jacek
On 05/11/2021 16:11, Jacek Caban wrote:
On 11/4/21 3:10 PM, Gabriel Ivăncescu wrote:
+ /* Create a temporary html element and parse it there */ + nsAString_InitDepend(&str, L"HTML"); + nsres = nsIDOMHTMLDocument_CreateElement(doc->nsdoc, &str, (nsIDOMElement**)&nshtml); + nsAString_Finish(&str); + if(NS_FAILED(nsres)) + return map_nsresult(nsres); + + if(name_len == 4 && !wcsnicmp(tag + 1, L"HTML", 4)) { + FIXME("Returning <html> element with no attributes\n"); +*ret = (nsIDOMElement*)nshtml; + return S_OK; + }
Did you try using <template> element instead of <html>? I'd expect it do do the right thing, but I didn't try.
I tried it now, it doesn't seem to work properly for all tags because it's context-dependent. It works fine for tags under <body>, but it won't work for the tags outside of it, such as <body> <head> <title> etc It seems they are simply discarded and the outer HTML looks like just a <template></template>. The html tag already has this issue but only with the <html> element which is probably not important (prints a FIXME anyway in such case).
+ + nsAString_InitDepend(&str, tag); + nsres = nsIDOMHTMLElement_SetInnerHTML(nshtml, &str); + nsAString_Finish(&str); + if(NS_FAILED(nsres)) { + hres = map_nsresult(nsres); + goto fail; + } + + /* Get the element and remove it from the temporary */ + if(!(p = heap_alloc((name_len + 1) * sizeof(WCHAR)))) + hres = E_OUTOFMEMORY; + else { + memcpy(p, tag + 1, name_len * sizeof(WCHAR)); + p[name_len] = '\0'; + nsAString_InitDepend(&str, p); + nsres = nsIDOMHTMLElement_GetElementsByTagName(nshtml, &str, &nscol); + nsAString_Finish(&str); + heap_free(p); + if(NS_FAILED(nsres)) + goto fail;
That's making things more complicated that they need to be. You could just use firstElementChild or something similar.
Unfortunately, it's not that simple. Continuing the above, when used in a <html> element, the inserted element tag can be in different places. For example, the innerHTML of such with a <div> would look like this: <head></head><body><div a="b"></div></body> This is what gecko does when setInnerHTML to a <div a="b">. But if the created element is, say, <head a="b">, it would be: <head a="b"></head><body></body> If it's <title a="b"> it would be: <head><title a="b"></title></head><body></body> That is, gecko places it in appropriate place depending on its context. As you can see, it's in different places, so how can I retrieve it without knowing its name? Do you have any ideas? Thanks, Gabriel
I thought of another idea, which I don't think is necessarily better, but maybe it's worth a thought. We could iterate through a bunch of root context tags, namely <template>, <head> and <html> in that order, then use setInnerHTML on them and retrieve the first child, until we get a child and then use that if we did. I guess <html> context might be tricky here, since it can be either <head> or <body> tag that is parsed, might need some special casing (and retrieve either first or second child in such case, perhaps we can just check the first letter since other tags should already work in either <template> or <head> themselves—so would have been filtered already). Just an idea. Is it worth pursuing?
On 11/6/21 2:46 PM, Gabriel Ivăncescu wrote:
I thought of another idea, which I don't think is necessarily better, but maybe it's worth a thought.
We could iterate through a bunch of root context tags, namely <template>, <head> and <html> in that order, then use setInnerHTML on them and retrieve the first child, until we get a child and then use that if we did.
I guess <html> context might be tricky here, since it can be either <head> or <body> tag that is parsed, might need some special casing (and retrieve either first or second child in such case, perhaps we can just check the first letter since other tags should already work in either <template> or <head> themselves—so would have been filtered already).
Just an idea. Is it worth pursuing?
The whole thing is still too hacky, in my opinion. If we can't get Gecko to do what we need, maybe we need to do parsing ourselves. Given that we only need to parse a small subset of HTML, it shouldn't be too bad and all we need from Gecko is createElement() and setAttribute(). Thanks, Jacek
On 06/11/2021 21:19, Jacek Caban wrote:
On 11/6/21 2:46 PM, Gabriel Ivăncescu wrote:
I thought of another idea, which I don't think is necessarily better, but maybe it's worth a thought.
We could iterate through a bunch of root context tags, namely <template>, <head> and <html> in that order, then use setInnerHTML on them and retrieve the first child, until we get a child and then use that if we did.
I guess <html> context might be tricky here, since it can be either <head> or <body> tag that is parsed, might need some special casing (and retrieve either first or second child in such case, perhaps we can just check the first letter since other tags should already work in either <template> or <head> themselves—so would have been filtered already).
Just an idea. Is it worth pursuing?
The whole thing is still too hacky, in my opinion. If we can't get Gecko to do what we need, maybe we need to do parsing ourselves. Given that we only need to parse a small subset of HTML, it shouldn't be too bad and all we need from Gecko is createElement() and setAttribute().
Thanks,
Jacek
Hi Jacek, I think parsing it ourselves might be somewhat complicated, because of stuff like the HTML escapes (e.g. " " & and so on), which would have to be handled as the tests show. A mixed way would be a simpler version of the first patch that goes roughly like the following. For this case, let's assume we want to create the <body a="b"> element, so: The first two steps are needed regardless of whether we parse it ourselves or not: 1) Parse its tag name ("body") 2) Create a <body> element Then: 3) Create a <template> element, and setInnerHTML to <foo a="b"> 4) Get its first child 5) Loop through all attributes on the child and set them on the element we created in (2) This is pretty much like first patch but simplified to <template> and allows gecko to parse it for us. Do you think it's feasible? If not, do you have some suggestions how I should implement the escapes? I guess a table? Thanks, Gabriel
On 11/7/21 2:38 PM, Gabriel Ivăncescu wrote:
On 06/11/2021 21:19, Jacek Caban wrote:
On 11/6/21 2:46 PM, Gabriel Ivăncescu wrote:
I thought of another idea, which I don't think is necessarily better, but maybe it's worth a thought.
We could iterate through a bunch of root context tags, namely <template>, <head> and <html> in that order, then use setInnerHTML on them and retrieve the first child, until we get a child and then use that if we did.
I guess <html> context might be tricky here, since it can be either <head> or <body> tag that is parsed, might need some special casing (and retrieve either first or second child in such case, perhaps we can just check the first letter since other tags should already work in either <template> or <head> themselves—so would have been filtered already).
Just an idea. Is it worth pursuing?
The whole thing is still too hacky, in my opinion. If we can't get Gecko to do what we need, maybe we need to do parsing ourselves. Given that we only need to parse a small subset of HTML, it shouldn't be too bad and all we need from Gecko is createElement() and setAttribute().
Thanks,
Jacek
Hi Jacek,
I think parsing it ourselves might be somewhat complicated, because of stuff like the HTML escapes (e.g. " " & and so on), which would have to be handled as the tests show.
A mixed way would be a simpler version of the first patch that goes roughly like the following. For this case, let's assume we want to create the <body a="b"> element, so:
The first two steps are needed regardless of whether we parse it ourselves or not:
1) Parse its tag name ("body") 2) Create a <body> element
Then:
3) Create a <template> element, and setInnerHTML to <foo a="b"> 4) Get its first child 5) Loop through all attributes on the child and set them on the element we created in (2)
This is pretty much like first patch but simplified to <template> and allows gecko to parse it for us.
Do you think it's feasible? If not, do you have some suggestions how I should implement the escapes? I guess a table?
Yeah, it's a bit better. It's not perfect, but it should work. Thanks, Jacek
On 08/11/2021 20:46, Jacek Caban wrote:
On 11/7/21 2:38 PM, Gabriel Ivăncescu wrote:
On 06/11/2021 21:19, Jacek Caban wrote:
On 11/6/21 2:46 PM, Gabriel Ivăncescu wrote:
I thought of another idea, which I don't think is necessarily better, but maybe it's worth a thought.
We could iterate through a bunch of root context tags, namely <template>, <head> and <html> in that order, then use setInnerHTML on them and retrieve the first child, until we get a child and then use that if we did.
I guess <html> context might be tricky here, since it can be either <head> or <body> tag that is parsed, might need some special casing (and retrieve either first or second child in such case, perhaps we can just check the first letter since other tags should already work in either <template> or <head> themselves—so would have been filtered already).
Just an idea. Is it worth pursuing?
The whole thing is still too hacky, in my opinion. If we can't get Gecko to do what we need, maybe we need to do parsing ourselves. Given that we only need to parse a small subset of HTML, it shouldn't be too bad and all we need from Gecko is createElement() and setAttribute().
Thanks,
Jacek
Hi Jacek,
I think parsing it ourselves might be somewhat complicated, because of stuff like the HTML escapes (e.g. " " & and so on), which would have to be handled as the tests show.
A mixed way would be a simpler version of the first patch that goes roughly like the following. For this case, let's assume we want to create the <body a="b"> element, so:
The first two steps are needed regardless of whether we parse it ourselves or not:
1) Parse its tag name ("body") 2) Create a <body> element
Then:
3) Create a <template> element, and setInnerHTML to <foo a="b"> 4) Get its first child 5) Loop through all attributes on the child and set them on the element we created in (2)
This is pretty much like first patch but simplified to <template> and allows gecko to parse it for us.
Do you think it's feasible? If not, do you have some suggestions how I should implement the escapes? I guess a table?
Yeah, it's a bit better. It's not perfect, but it should work.
Thanks,
Jacek
So I decided to go with contextual fragment instead, setInnerHTML seemed to be a bit fragile/buggy or maybe I'm not understanding the issues, but getFirstChild or getFirstElementChild always returned NULL after setInnerHTML, despite getInnerHTML showing the proper html when tracing. I considered setOuterHTML but that seems to not work at all, no matter what I input to it. It doesn't do anything. I then looked at our implementation of outerHTML, and it seems to be using contextual fragments to replace the node, so that's what I went with. I think it's cleaner too, since we don't use a <template> tag anymore (or any other tag). Note that even contextual fragments are still context-dependent (as the name shows) so I still had to use the same tricks with dummy tag / attribute copying as with innerHTML, unfortunately. But at least now it's correct in *all* cases.
For IE modes 8 and above, IHTMLElement::setAttribute uses the underlying nsIDOMElement::SetAttribute, which does nothing since we're already enumerating the ns attributes, and we're supposed to create the DISPIDs for them. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlelem.c | 5 ++++- dlls/mshtml/tests/documentmode.js | 2 -- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c index 503a287..6624264 100644 --- a/dlls/mshtml/htmlelem.c +++ b/dlls/mshtml/htmlelem.c @@ -6480,7 +6480,10 @@ static HRESULT HTMLElement_populate_props(DispatchEx *dispex) } else V_BSTR(&value) = NULL; - IHTMLElement_setAttribute(&This->IHTMLElement_iface, name, value, 0); + hres = IDispatchEx_GetDispID(&dispex->IDispatchEx_iface, name, fdexNameEnsure | fdexNameCaseInsensitive, &id); + if(SUCCEEDED(hres)) + set_elem_attr_value_by_dispid(This, id, &value); + SysFreeString(name); VariantClear(&value); } diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 69f4796..d7c1350 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 { -- 2.31.1
participants (3)
-
Gabriel Ivăncescu -
Jacek Caban -
Marvin