Bug 58032

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

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

Created attachment 88990 [details] proposed fix Simon or Jocelyn could you confirm whether this is the correct behaviour of the API?

Comment on attachment 88990 [details] proposed fix This is a progression IMO, so r=me. :)

Comment on attachment 88990 [details] proposed fix Clearing flags on attachment: 88990 Committed r83440: <http://trac.webkit.org/changeset/83440>

All reviewed patches have been landed. Closing bug.

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?

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

Is this the same bug as <https://bugs.webkit.org/show_bug.cgi?id=60007>?

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

As described in my previous comment, the regression itself was fixed. To describe the real problem I've opened bug 60372.

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

Created attachment 92617 [details] second fix (basically reverts the previous change)

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

(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

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.