Summary: | [Qt] QWebElement::attribute always returns empty result for input's values | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy Nichols <andy.nichols> | ||||||||||||||||||||||||||||||||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED INVALID | ||||||||||||||||||||||||||||||||||||
Severity: | Major | CC: | benjamin, cmarcelo, douaberi, hausmann, kent.hansen, max.hong.shen, mikelupow, robert, tonikitoo, webkit.review.bot, wolfram | ||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Please follow the QtWebKit bug reporting guidelines when reporting bugs. See http://trac.webkit.org/wiki/QtWebKitBugs Specifically: - The 'QtWebKit' component should only be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit http://trac.webkit.org/wiki/QtWebKitBugs#Component - Add the keyword 'Qt' to signal that it's a Qt-related bug http://trac.webkit.org/wiki/QtWebKitBugs#Keywords Reproduced against r55658. On the initial click, hasAttribute("value") returns false, and attributeNames() returns only "id" and "name". setAttribute("value", ...) does cause the element to gain a "value" attribute, but it seems to be completely detached from the value of the input field. That is because some HTMLInputElement (e.g. text input element) stores its value separately from element's attribute map, which means each value has two copies. For QWebElement, it only can access the values in the element's attribute map. Not sure whether it is a qtwebkit bug or "features" for security reasons. (In reply to comment #2) > Reproduced against r55658. > On the initial click, hasAttribute("value") returns false, and attributeNames() returns only "id" and "name". > setAttribute("value", ...) does cause the element to gain a "value" attribute, but it seems to be completely detached from the value of the input field. (In reply to comment #3) > That is because some HTMLInputElement (e.g. text input element) stores its value separately from element's attribute map, which means each value has two copies. > > For QWebElement, it only can access the values in the element's attribute map. > > Not sure whether it is a qtwebkit bug or "features" for security reasons. I think there is method in the madness and that QWebElement just needs extra API specifically for the current value in an input element. We don't want to modify the behaviour of attribute() because we might still want access to the initial value defined by the page. Created attachment 72454 [details]
Patch
Comment on attachment 72454 [details]
Patch
Isn't currentValue something like computeValue?
(In reply to comment #6) > (From update of attachment 72454 [details]) > Isn't currentValue something like computeValue? Don't think so. AFAICT it's: String HTMLInputElement::value() const Comment on attachment 72454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72454&action=review r- for minor issue, otherwise LGTM. In all cases, needs to be coordinated with Oslo as this involves API addition. > WebKit/qt/Api/qwebelement.cpp:400 > +QString QWebElement::currentValue() const New API function: require \since tag. *** Bug 48230 has been marked as a duplicate of this bug. *** *** Bug 53319 has been marked as a duplicate of this bug. *** I would actually change ::attribute() and ::setAttribute() to work on the input value and not the original value. Doing otherwise would give unexpected result from what one would expect. See #53319 for example. Created attachment 89729 [details]
attribute/setAttribute patch to be compatible with html inputs' values
Attachment 89729 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 89729 [details] attribute/setAttribute patch to be compatible with html inputs' values View in context: https://bugs.webkit.org/attachment.cgi?id=89729&action=review R-: Missing changelog, and missing tests. The idea look sane though, is value the only attribute behaving like that? > B/qwebelement.cpp:127 > + Empty line? > R-: Missing changelog, How to add it? > and missing tests. I think that no new tests are required but existing (WebKit/qt/tests/qwebelement/tst_qwebelement.cpp) will work fine. > The idea look sane though, is value the only attribute behaving like that? Yes, only values of html inputs are broken > Empty line? Just a typo. (In reply to comment #15) > > R-: Missing changelog, > How to add it? Tools/Scripts/prepare-ChangeLog --bug XXX --git-commit=SHA Created attachment 90906 [details]
New patch, with "checked" attribute support
Comment on attachment 90906 [details] New patch, with "checked" attribute support Rejecting attachment 90906 [details] from review queue. wolfram@ritsuka.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights. Comment on attachment 90906 [details] New patch, with "checked" attribute support Rejecting attachment 90906 [details] from commit-queue. wolfram@ritsuka.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. Attachment 90906 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebelement.cpp', u'S..." exit_code: 1
Source/WebKit/qt/Api/qwebelement.cpp:386: This { should be at the end of the previous line [whitespace/braces] [4]
Source/WebKit/qt/Api/qwebelement.cpp:391: This { should be at the end of the previous line [whitespace/braces] [4]
Source/WebKit/qt/Api/qwebelement.cpp:392: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Source/WebKit/qt/Api/qwebelement.cpp:425: This { should be at the end of the previous line [whitespace/braces] [4]
Total errors found: 4 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 90906 [details] did not build on qt: Build output: http://queues.webkit.org/results/8508200 Created attachment 90907 [details]
style fixed
Attachment 90907 [details] did not build on qt: Build output: http://queues.webkit.org/results/8504627 Created attachment 90908 [details]
warning fixed
Comment on attachment 90908 [details] warning fixed View in context: https://bugs.webkit.org/attachment.cgi?id=90908&action=review I think we need tests, or if there are existing ones, please say it in the ChangeLog entry. > Source/WebKit/qt/ChangeLog:1 > +2011-04-25 <> Fix it, please Created attachment 91244 [details]
changelog and tests added
Comment on attachment 91244 [details]
changelog and tests added
Sorry, wrong file
Created attachment 91245 [details]
changelog and tests added
Attachment 91245 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebelement.cpp', u'S..." exit_code: 1
Last 3072 characters of output:
ce indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1063: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1064: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1065: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1066: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1067: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1068: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1071: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1072: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1073: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1074: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1075: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1076: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1077: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1078: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1079: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1080: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1081: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1082: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1083: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 35 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 91251 [details]
changelog and tests added, style fixed
Attachment 91245 [details] did not build on qt: Build output: http://queues.webkit.org/results/8507893 Created attachment 91252 [details]
changelog and tests added, style fixed, typo fixed
Comment on attachment 91252 [details] changelog and tests added, style fixed, typo fixed View in context: https://bugs.webkit.org/attachment.cgi?id=91252&action=review Still one minor request: > Source/WebKit/qt/Api/qwebelement.cpp:391 > + htmlElement->setChecked(checked, true); Please make this line like: htmlElement->setChecked(checked, true /*explain shortly here, maybe in one or two words, what this boolean means*/); Created attachment 91626 [details]
Short comment added
Attachment 91626 [details] did not build on qt: Build output: http://queues.webkit.org/results/8516714 Created attachment 91636 [details]
fix
Created attachment 91644 [details]
I'm awfully sorry for all these junk files
I'm awfully sorry, but... any news? Comment on attachment 91644 [details] I'm awfully sorry for all these junk files View in context: https://bugs.webkit.org/attachment.cgi?id=91644&action=review > Source/WebKit/qt/Api/qwebelement.cpp:392 > + HTMLInputElement* htmlElement = static_cast<HTMLInputElement*>(m_element); > + if (HTMLNames::valueAttr == name) > + htmlElement->setValue(value, true /* sendChangeEvent */); > + else if (HTMLNames::checkedAttr == name) { > + bool checked = !QString::compare(value, QString::fromLatin1("checked"), Qt::CaseInsensitive); > + htmlElement->setChecked(checked, true /* sendChangeEvent */); > + } Invalid indentation. > Source/WebKit/qt/Api/qwebelement.cpp:428 > + if (m_element->hasTagName(HTMLNames::inputTag)) { > + HTMLInputElement* htmlElement = static_cast<HTMLInputElement*>(m_element); > + if (HTMLNames::valueAttr == name) > + return htmlElement->value(); > + else if (HTMLNames::checkedAttr == name) > + return htmlElement->checked() ? QString::fromLatin1("checked") : QString(); > + } This is not indented correctly. > Source/WebKit/qt/ChangeLog:1 > +2011-04-29 wolfy <wolfram@ritsuka.org> You should probably put your name in here. > Source/WebKit/qt/ChangeLog:15 > + [Qt] QWebElement::attribute always returns empty result for input's values > + https://bugs.webkit.org/show_bug.cgi?id=32865 This, the title and url, usually goes above the description, not after. > Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1045 > + // No initial value You should have a period at the end of sentences. (This one and the other comments). > Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1049 > + QCOMPARE(inputElement.attribute("value"), QString()); I would think the correct value for inputElement.attribute("value") is empty but not null. While a null string would be if there is no value attribute. > Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1076 > + QCOMPARE(inputElement.attribute("value"), QString("")); Kind of reverse from the comment for the text, I would expect value to be null in this case, not empty. (In reply to comment #38) > I'm awfully sorry, but... any news? It happens a patch is ignored, especially when it had lots of update for no good reason. A good way out of the problem is to poke reviewers on IRC. Please come on #qtwebkit (and/or #webkit), and if you see your patch is not going forward, ask the reviewers for advices on how to improve the patch. I am not sure your patch is correct yet, because the solution is maybe to change Element::value(). Other than that, the code and test looks sane. Just a few more iterations to get it to be perfect. Created attachment 94570 [details] Patch refined according to last comment (In reply to comment #39) > Invalid indentation. Fixed. > This is not indented correctly. Fixed. > > Source/WebKit/qt/ChangeLog:1 > > +2011-04-29 wolfy <wolfram@ritsuka.org> > > You should probably put your name in here. I don't want to do that. > This, the title and url, usually goes above the description, not after. Fixed. > > Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1045 > > + // No initial value > > You should have a period at the end of sentences. (This one and the other comments). Fixed. But I didn't see periods in any other comments in this file. > I would think the correct value for inputElement.attribute("value") is empty but not null. > While a null string would be if there is no value attribute. > > Kind of reverse from the comment for the text, I would expect value to be null in this case, not empty. Fixed. Attachment 94570 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebelement.cpp', u'S..." exit_code: 1
Source/WebKit/qt/Api/qwebelement.cpp:425: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 94576 [details]
Style fixed
Attachment 94576 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebelement.cpp', u'S..." exit_code: 1
Source/WebKit/qt/Api/qwebelement.cpp:425: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Source/WebKit/qt/Api/qwebelement.cpp:429: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 94578 [details]
Style fixed
(In reply to comment #11) > I would actually change ::attribute() and ::setAttribute() to work on the input value and not the original value. > > Doing otherwise would give unexpected result from what one would expect. See #53319 for example. Ok, so we have to distinguish between HTML attributes and DOM property attributes ( http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-642250288 ) The current implementation of setAttribute() and attribute() corresponds to HTML attributes, and the namespace "overloads" of these functions stress this. So the current behaviour is very well defined and I don't think we can change it without breaking the *NS() overloads or introducing inconsistencies. Consequently I think what we need here are functions to change the DOM property attributes. The most logical way I think to implement this would be to add this when we change to V8 and introduce QtScript as dependency in the API. Then we can have property setters/getters that correspond to properties in JavaScript, which are a 1:1 mapping to DOM property attributes. Comment on attachment 94578 [details]
Style fixed
I'm going to say r- because of the previous comment in the sense that this makes setAttribute() inconsistent to setAttributeNS() (and the same for the getters).
Unless someone comes up with a nicer proposal I'd suggest we solve this when we introduce the QtScript dependency. Then we can have for example
QScriptValue QWebElement::scriptObject()
and you can do
element.setAttribute("value", "this is the initial value");
QScriptValue jsobject = element.scriptObject();
jsobject.setProperty("value", "current text");
Ok, it's very interesting idea, but... could you give us any quick advice which will allow to work with html inputs? They are mostly unusable now. (In reply to comment #48) > Ok, it's very interesting idea, but... could you give us any quick advice which will allow to work with html inputs? They are mostly unusable now. If you're just looking for a workaround, here's what you could try today (untested): QString value = element->evaluateJavaScript("this.value"); ... element->evaluateJavaScript("this.value = " + newValue); It's not a solution for me. JS calls are very slow, so I'll continue to use my patch. Could you make some approximations about times when fix proposed by you will be done? === Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines. |
Created attachment 45380 [details] Example to demonstrat problem How to reproduce the bug: 1) Create simple application with QWebView and button. Add slot for button click event with code like the following: void onTestClick() { QWebFrame* curFrame = m_webview->page()->currentFrame(); if (curFrame) { QWebElement elem = curFrame->documentElement().findFirst("#login"); if (!elem.isNull()) { QString ret = elem.attribute("value", "nothing :("); QMessageBox::information(0, "Result", ret, QMessageBox::Ok); } } } 2) Load any web page which contains <input> element with attribute "id" defined (for example <input id="login" name="login"/>) 3) Enter any text to this field and press button. Messagebox will be shown but displayed text always will be "nothing :(" Note that if you inject JQuery to the page in slot connected to loadFinished() signal and place the following code to onTestClick(): QString ret = m_webview->evaluateJavaScript("$('#login').attr('value')").toString(); QMessageBox::information(0, "Result", ret, QMessageBox::Ok); value will be extracted correctly.