58032 2011-04-07 05:47:31 -0700 [Qt] REGRESSION(83122): tst_QWebElement::style() fails 2011-05-06 13:51:30 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC All RESOLVED FIXED Qt, QtTriaged P2 Major --- 38654 1 ossy ademar abecsi ademar cmarcelo commit-queue hausmann ian jturcotte koivisto oldest_to_newest 381255 0 ossy 2011-04-07 05:47:31 -0700 FAIL! : tst_QWebElement::style() Compared values are not the same Actual (p.styleProperty("color", QWebElement::InlineStyle)): blue Expected (QLatin1String("green")): green Loc: [/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp(486)] 381381 1 abecsi 2011-04-07 08:53:30 -0700 (In reply to comment #0) > FAIL! : tst_QWebElement::style() Compared values are not the same > Actual (p.styleProperty("color", QWebElement::InlineStyle)): blue > Expected (QLatin1String("green")): green > Loc: [/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp(486)] Seems that the !important rule is not handled correctly. Need to check whether this only affects the Qt API. 383104 2 abecsi 2011-04-11 04:57:58 -0700 (In reply to comment #1) > (In reply to comment #0) > > FAIL! : tst_QWebElement::style() Compared values are not the same > > Actual (p.styleProperty("color", QWebElement::InlineStyle)): blue > > Expected (QLatin1String("green")): green > > Loc: [/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp(486)] > > Seems that the !important rule is not handled correctly. > Need to check whether this only affects the Qt API. The documentation of QWebElement::styleProperty describes the following for the QWebElement::StyleResolveStrategy enum's InlineStyle: "Return the property value as it is defined in the element, without respecting style inheritance and other CSS rules." So I think the result is actually correct and r83122 fixed this behaviour. 383109 3 88990 abecsi 2011-04-11 05:02:05 -0700 Created attachment 88990 proposed fix Simon or Jocelyn could you confirm whether this is the correct behaviour of the API? 383133 4 88990 kling 2011-04-11 06:17:30 -0700 Comment on attachment 88990 proposed fix This is a progression IMO, so r=me. :) 383146 5 88990 commit-queue 2011-04-11 06:45:10 -0700 Comment on attachment 88990 proposed fix Clearing flags on attachment: 88990 Committed r83440: <http://trac.webkit.org/changeset/83440> 383147 6 commit-queue 2011-04-11 06:45:18 -0700 All reviewed patches have been landed. Closing bug. 388154 7 ossy 2011-04-18 16:21:16 -0700 before this change: -------------------- FAIL! : tst_QWebElement::style() Compared values are not the same Actual (p.styleProperty("color", QWebElement::InlineStyle)): blue Expected (QLatin1String("green")): green Loc: [/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp(486)] after this change: ------------------- FAIL! : tst_QWebElement::style() Compared values are not the same Actual (p.styleProperty("color", QWebElement::CascadedStyle)): yellow Expected (QLatin1String("green")): green Loc: [/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp(487)] Reopen because of a very strange bug. :o I don't understand how is it possible. It seems if an expected result modification make the next test case fail. Could you check it? 397461 8 ademar 2011-05-03 15:35:36 -0700 (In reply to comment #7) > I don't understand how is it possible. It seems if an > expected result modification make the next test case fail. You didn't see the second failure before because the test was failing (and therefore aborting) before reaching it. If you comment-out the first call (the one fixed by this patch), you'll see that the second error was there already (also introduced by r83122) Anyway, Ossy is right: this bug is still open, as a regression is still there. @Andras: do you plan to work more on it? 397463 9 ian 2011-05-03 15:38:52 -0700 Is this the same bug as <https://bugs.webkit.org/show_bug.cgi?id=60007>? 397908 10 ademar 2011-05-04 07:18:05 -0700 (In reply to comment #9) > Is this the same bug as <https://bugs.webkit.org/show_bug.cgi?id=60007>? What a mess :-P Now we are back to our original failure, but the second test pass: """ FAIL! : tst_QWebElement::style() Compared values are not the same Actual (p.styleProperty("color", QWebElement::InlineStyle)): green Expected (QLatin1String("blue")): blue Loc: [/opt/projects/webkit/webkit/Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp(486)] """ If I comment-out this line, the second test passes. So, we are back to the original problem and the fix requires a change in our Qt implementation, because as pointed out by Andras, the Qt API documentation is clear about QWebElement::InlineStyle: "Return the property value as it is defined in the element, without respecting style inheritance and other CSS rules", which is exactly what we test now. In other words, our QWebElement::InlineStyle never worked: the test was wrong and r83122 introduced a bug that made us think it was actually fixing our API and thus we fixed our test. When that bug was fixed, we returned to our buggy API, but this time with the right test. :-) Andras: do you plan to work on this? If not, I would like to open a different bug (which is basically: "QWebElement::InlineStyle doesn't work as expected") and try to fix it. 399506 11 ademar 2011-05-06 07:38:22 -0700 As described in my previous comment, the regression itself was fixed. To describe the real problem I've opened bug 60372. 399681 12 ademar 2011-05-06 11:48:08 -0700 OK, I think I finally figured it out: From https://bugs.webkit.org/show_bug.cgi?id=60372#c1 WebCore::CSSMutableStyleDeclaration::setProperty() has this: """ // When replacing an existing property value, this moves the property to the end of the list. """ (it also says "Firefox preserves the position, and MSIE moves the property to the beginning", but that's not relevant for this bug) And in the test code, we set the property twice for the same element: p.setStyleProperty("color", "green !important"); ... p.setStyleProperty("color", "blue"); ... So when we call styleProperty(QWebElement::InlineStyle) after the second setStyleProperty(), there are two inline styles and we get the one with high priority. So in summary, the implementation is right and the test is wrong... :P Patch is on the way 399686 13 92617 ademar 2011-05-06 11:50:53 -0700 Created attachment 92617 second fix (basically reverts the previous change) 399701 14 cmarcelo 2011-05-06 12:09:03 -0700 (In reply to comment #12) > p.setStyleProperty("color", "green !important"); > ... > p.setStyleProperty("color", "blue"); > ... > > So when we call styleProperty(QWebElement::InlineStyle) after the second setStyleProperty(), there are two inline styles and we get the one with high priority. For some reason I was expecting the second call to replace the color property, set it to a new value, instead of appending a new inline style :-/ is really appending the behavior we want? 399718 15 ademar 2011-05-06 12:20:26 -0700 (In reply to comment #14) > (In reply to comment #12) > > p.setStyleProperty("color", "green !important"); > > ... > > p.setStyleProperty("color", "blue"); > > ... > > > > So when we call styleProperty(QWebElement::InlineStyle) after the second setStyleProperty(), there are two inline styles and we get the one with high priority. > > For some reason I was expecting the second call to replace the color property, set it to a new value, instead of appending a new inline style :-/ is really appending the behavior we want? Me too. But now re-reading the code more carefully, I think it's not actually duplicating, it's just moving it to the end of the list. /me needs coffee and a debug build of qtwebkit... what a confusing problem. :P 399787 16 ademar 2011-05-06 13:51:30 -0700 I'm very sorry for the noise... Please ignore my previous comments. This bug is closed and the bug to be solved is really bug 60372 :-P I'll make sure my patch include documentation improvements. 88990 2011-04-11 05:02:05 -0700 2011-04-18 16:21:37 -0700 proposed fix tst_qwebelement.patch text/plain 1637 abecsi ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvcXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9x dC9DaGFuZ2VMb2cKaW5kZXggMjAxODlkNi4uNWNjNjUwZCAxMDA2NDQKLS0tIGEvU291cmNlL1dl YktpdC9xdC9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9xdC9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAxMS0wNC0xMSAgQW5kcmFzIEJlY3NpICA8YWJlY3NpQHdlYmtpdC5vcmc+ CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW1F0XSBS RUdSRVNTSU9OKDgzMTIyKTogdHN0X1FXZWJFbGVtZW50OjpzdHlsZSgpIGZhaWxzCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01ODAzMgorCisgICAgICAg IEFjY29yZGluZyB0byB0aGUgZG9jdW1lbnRhdGlvbiBvZiBRV2ViRWxlbWVudCB0aGUgc3R5bGVQ cm9wZXJ0eSBtZXRob2Qgc2hvdWxkCisgICAgICAgIG5vdCByZXNwZWN0IHN0eWxlIGluaGVyaXRh bmNlIGFuZCBvdGhlciBDU1MgcnVsZXMgZm9yIHRoZSBJbmxpbmVTdHlsZSBlbnVtLgorCisgICAg ICAgIHI4MzEyMiBmaXhlZCB0aGlzIGJlaGF2aW91ci4KKworICAgICAgICAqIHRlc3RzL3F3ZWJl bGVtZW50L3RzdF9xd2ViZWxlbWVudC5jcHA6CisgICAgICAgICh0c3RfUVdlYkVsZW1lbnQ6OnN0 eWxlKTogRml4IHRoZSBleHBlY3RlZCBjb2xvciBmb3IgUVdlYkVsZW1lbnQ6OklubGluZVN0eWxl LgorCiAyMDExLTA0LTA4ICBTaGVyaWZmIEJvdCAgPHdlYmtpdC5yZXZpZXcuYm90QGdtYWlsLmNv bT4KIAogICAgICAgICBVbnJldmlld2VkLCByb2xsaW5nIG91dCByODMyOTUuCmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViS2l0L3F0L3Rlc3RzL3F3ZWJlbGVtZW50L3RzdF9xd2ViZWxlbWVudC5jcHAg Yi9Tb3VyY2UvV2ViS2l0L3F0L3Rlc3RzL3F3ZWJlbGVtZW50L3RzdF9xd2ViZWxlbWVudC5jcHAK aW5kZXggYzdjNDlhMi4uOWU5OTQ4YSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9xdC90ZXN0 cy9xd2ViZWxlbWVudC90c3RfcXdlYmVsZW1lbnQuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvcXQv dGVzdHMvcXdlYmVsZW1lbnQvdHN0X3F3ZWJlbGVtZW50LmNwcApAQCAtNDgzLDcgKzQ4Myw3IEBA IHZvaWQgdHN0X1FXZWJFbGVtZW50OjpzdHlsZSgpCiAgICAgUUNPTVBBUkUocC5zdHlsZVByb3Bl cnR5KCJjb2xvciIsIFFXZWJFbGVtZW50OjpDYXNjYWRlZFN0eWxlKSwgUUxhdGluMVN0cmluZygi Z3JlZW4iKSk7CiAKICAgICBwLnNldFN0eWxlUHJvcGVydHkoImNvbG9yIiwgImJsdWUiKTsKLSAg ICBRQ09NUEFSRShwLnN0eWxlUHJvcGVydHkoImNvbG9yIiwgUVdlYkVsZW1lbnQ6OklubGluZVN0 eWxlKSwgUUxhdGluMVN0cmluZygiZ3JlZW4iKSk7CisgICAgUUNPTVBBUkUocC5zdHlsZVByb3Bl cnR5KCJjb2xvciIsIFFXZWJFbGVtZW50OjpJbmxpbmVTdHlsZSksIFFMYXRpbjFTdHJpbmcoImJs dWUiKSk7CiAgICAgUUNPTVBBUkUocC5zdHlsZVByb3BlcnR5KCJjb2xvciIsIFFXZWJFbGVtZW50 OjpDYXNjYWRlZFN0eWxlKSwgUUxhdGluMVN0cmluZygiZ3JlZW4iKSk7CiAKICAgICBwLnNldFN0 eWxlUHJvcGVydHkoImNvbG9yIiwgImJsdWUgIWltcG9ydGFudCIpOwo= 92617 2011-05-06 11:50:53 -0700 2011-05-06 12:14:26 -0700 second fix (basically reverts the previous change) 0001-Qt-REGRESSION-83122-tst_QWebElement-style-fails.patch text/plain 2586 ademar RnJvbSBhNjM5NWVmMWJkMDIyZDk4NzFhODFiMDhkMTk2YWI4YTZlZTQ3ZTJjIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBZGVtYXIgZGUgU291emEgUmVpcyBKciA8YWRlbWFyLnJlaXNA b3BlbmJvc3NhLm9yZz4KRGF0ZTogRnJpLCA2IE1heSAyMDExIDE1OjQ1OjQzIC0wMzAwClN1Ympl Y3Q6IFtQQVRDSF0gW1F0XSBSRUdSRVNTSU9OKDgzMTIyKTogdHN0X1FXZWJFbGVtZW50OjpzdHls ZSgpIGZhaWxzCgpodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTgwMzIK ClRoaXMgY29tbWl0IHJldmVydHMgdGhlIHByZXZpb3VzIGNoYW5nZSAocjgzNDQwKS4gVGhlIHRl c3Qgd2FzCm9yaWdpbmFsbHkgcmlnaHQuIHI4MzEyMiBpbnRyb2R1Y2VkIGEgcmVncmVzc2lvbiB3 aGljaCB3YXMgbGF0ZXIKZml4ZWQgaW4gcjg1NTg1LiBTZWUgYnVnIGNvbW1lbnRzIGZvciB0aGUg d2hvbGUgc3RvcnkuCgoqIHRlc3RzL3F3ZWJlbGVtZW50L3RzdF9xd2ViZWxlbWVudC5jcHA6Cih0 c3RfUVdlYkVsZW1lbnQ6OnN0eWxlKTogZml4IHRoZSB0ZXN0IGV4cGVjdGF0aW9uCi0tLQogU291 cmNlL1dlYktpdC9xdC9DaGFuZ2VMb2cgICAgICAgICAgICAgICAgICAgICAgICAgfCAgIDE0ICsr KysrKysrKysrKysrCiAuLi4vcXQvdGVzdHMvcXdlYmVsZW1lbnQvdHN0X3F3ZWJlbGVtZW50LmNw cCAgICAgICB8ICAgIDQgKysrLQogMiBmaWxlcyBjaGFuZ2VkLCAxNyBpbnNlcnRpb25zKCspLCAx IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvcXQvQ2hhbmdlTG9nIGIv U291cmNlL1dlYktpdC9xdC9DaGFuZ2VMb2cKaW5kZXggOWZiNzNlOS4uM2RlMGUxYSAxMDA2NDQK LS0tIGEvU291cmNlL1dlYktpdC9xdC9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9xdC9D aGFuZ2VMb2cKQEAgLTEsMyArMSwxNyBAQAorMjAxMS0wNS0wNiAgQWRlbWFyIGRlIFNvdXphIFJl aXMgSnIuICA8YWRlbWFyLnJlaXNAb3BlbmJvc3NhLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBbUXRdIFJFR1JFU1NJT04oODMxMjIpOiB0c3Rf UVdlYkVsZW1lbnQ6OnN0eWxlKCkgZmFpbHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTU4MDMyCisKKyAgICAgICAgVGhpcyBjb21taXQgcmV2ZXJ0cyB0 aGUgcHJldmlvdXMgY2hhbmdlIChyODM0NDApLiBUaGUgdGVzdCB3YXMKKyAgICAgICAgb3JpZ2lu YWxseSByaWdodC4gcjgzMTIyIGludHJvZHVjZWQgYSByZWdyZXNzaW9uIHdoaWNoIHdhcyBsYXRl cgorICAgICAgICBmaXhlZCBpbiByODU1ODUuIFNlZSBidWcgY29tbWVudHMgZm9yIHRoZSB3aG9s ZSBzdG9yeS4KKworICAgICAgICAqIHRlc3RzL3F3ZWJlbGVtZW50L3RzdF9xd2ViZWxlbWVudC5j cHA6CisgICAgICAgICh0c3RfUVdlYkVsZW1lbnQ6OnN0eWxlKTogZml4IHRoZSB0ZXN0IGV4cGVj dGF0aW9uCisKIDIwMTEtMDUtMDQgIFBoaWxpcHBlIE5vcm1hbmQgIDxwbm9ybWFuZEBpZ2FsaWEu Y29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IERpbWl0cmkgR2xhemtvdi4KZGlmZiAtLWdpdCBh L1NvdXJjZS9XZWJLaXQvcXQvdGVzdHMvcXdlYmVsZW1lbnQvdHN0X3F3ZWJlbGVtZW50LmNwcCBi L1NvdXJjZS9XZWJLaXQvcXQvdGVzdHMvcXdlYmVsZW1lbnQvdHN0X3F3ZWJlbGVtZW50LmNwcApp bmRleCA5ZTk5NDhhLi45YTFkN2JiIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L3F0L3Rlc3Rz L3F3ZWJlbGVtZW50L3RzdF9xd2ViZWxlbWVudC5jcHAKKysrIGIvU291cmNlL1dlYktpdC9xdC90 ZXN0cy9xd2ViZWxlbWVudC90c3RfcXdlYmVsZW1lbnQuY3BwCkBAIC00ODIsOCArNDgyLDEwIEBA IHZvaWQgdHN0X1FXZWJFbGVtZW50OjpzdHlsZSgpCiAgICAgUUNPTVBBUkUocC5zdHlsZVByb3Bl cnR5KCJjb2xvciIsIFFXZWJFbGVtZW50OjpJbmxpbmVTdHlsZSksIFFMYXRpbjFTdHJpbmcoImdy ZWVuIikpOwogICAgIFFDT01QQVJFKHAuc3R5bGVQcm9wZXJ0eSgiY29sb3IiLCBRV2ViRWxlbWVu dDo6Q2FzY2FkZWRTdHlsZSksIFFMYXRpbjFTdHJpbmcoImdyZWVuIikpOwogCisgICAgLy8gYSBz ZWNvbmQgY29sb3IgcHJvcGVydHkgc2V0LCBib3RoIHdpbGwgYmUga2VwdCBpbmxpbmUsIGJ1dCB0 aGUgIWltcG9ydGFudAorICAgIC8vIHdpbGwgYmUgdXNlZC4KICAgICBwLnNldFN0eWxlUHJvcGVy dHkoImNvbG9yIiwgImJsdWUiKTsKLSAgICBRQ09NUEFSRShwLnN0eWxlUHJvcGVydHkoImNvbG9y IiwgUVdlYkVsZW1lbnQ6OklubGluZVN0eWxlKSwgUUxhdGluMVN0cmluZygiYmx1ZSIpKTsKKyAg ICBRQ09NUEFSRShwLnN0eWxlUHJvcGVydHkoImNvbG9yIiwgUVdlYkVsZW1lbnQ6OklubGluZVN0 eWxlKSwgUUxhdGluMVN0cmluZygiZ3JlZW4iKSk7CiAgICAgUUNPTVBBUkUocC5zdHlsZVByb3Bl cnR5KCJjb2xvciIsIFFXZWJFbGVtZW50OjpDYXNjYWRlZFN0eWxlKSwgUUxhdGluMVN0cmluZygi Z3JlZW4iKSk7CiAKICAgICBwLnNldFN0eWxlUHJvcGVydHkoImNvbG9yIiwgImJsdWUgIWltcG9y dGFudCIpOwotLSAKMS43LjMuNAoK