Bug 32865 – [Qt] QWebElement::attribute always returns empty result for input's values

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Andy Nichols]

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.

[Tor Arne Vestbø]

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

[Kent Hansen]

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.

[Yi Shen]

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.

[Robert Hogan]

(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.

[Robert Hogan]

Created attachment 72454 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 72454 [details]
Patch

Isn't currentValue something like computeValue?

[Robert Hogan]

(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

[Ariya Hidayat]

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.

[Benjamin Poulain]

*** Bug 48230 has been marked as a duplicate of this bug. ***

[Benjamin Poulain]

*** Bug 53319 has been marked as a duplicate of this bug. ***

[Benjamin Poulain]

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.

[wolfy]

Created attachment 89729 [details]
attribute/setAttribute patch to be compatible with html inputs' values

[WebKit Review Bot]

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.

[Benjamin Poulain]

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?

[wolfy]

> 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.

[Antonio Gomes]

(In reply to comment #15)
> > R-: Missing changelog, 
> How to add it?

Tools/Scripts/prepare-ChangeLog --bug XXX --git-commit=SHA

[wolfy]

Created attachment 90906 [details]
New patch, with "checked" attribute support

[WebKit Review Bot]

Comment on attachment 90906 [details]
New patch, with "checked" attribute support

Rejecting attachment 90906 [details] from review queue.

[e-mail] 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.

[WebKit Review Bot]

Comment on attachment 90906 [details]
New patch, with "checked" attribute support

Rejecting attachment 90906 [details] from commit-queue.

[e-mail] 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.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 90906 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8508200

[wolfy]

Created attachment 90907 [details]
style fixed

[Early Warning System Bot]

Attachment 90907 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8504627

[wolfy]

Created attachment 90908 [details]
warning fixed

[Antonio Gomes]

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
> [phone]    <>

Fix it, please

[wolfy]

Created attachment 91244 [details]
changelog and tests added

[wolfy]

Comment on attachment 91244 [details]
changelog and tests added

Sorry, wrong file

[wolfy]

Created attachment 91245 [details]
changelog and tests added

[WebKit Review Bot]

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.

[wolfy]

Created attachment 91251 [details]
changelog and tests added, style fixed

[Early Warning System Bot]

Attachment 91245 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8507893

[wolfy]

Created attachment 91252 [details]
changelog and tests added, style fixed, typo fixed

[Antonio Gomes]

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*/);

[wolfy]

Created attachment 91626 [details]
Short comment added

[Early Warning System Bot]

Attachment 91626 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8516714

[wolfy]

Created attachment 91636 [details]
fix

[wolfy]

Created attachment 91644 [details]
I'm awfully sorry for all these junk files

[wolfy]

I'm awfully sorry, but... any news?

[Benjamin Poulain]

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
> [phone]  wolfy  <[e-mail]>

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.

[Benjamin Poulain]

(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.

[wolfy]

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
> > [phone]  wolfy  <[e-mail]>
> 
> 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.

[WebKit Review Bot]

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.

[wolfy]

Created attachment 94576 [details]
Style fixed

[WebKit Review Bot]

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.

[wolfy]

Created attachment 94578 [details]
Style fixed

[Simon Hausmann]

(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.

[Simon Hausmann]

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");

[wolfy]

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.

[Simon Hausmann]

(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);

[wolfy]

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?

[Jocelyn Turcotte]

=== 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.