Bug 58032 – [Qt] REGRESSION(83122): tst_QWebElement::style() fails

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

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)]

[Andras Becsi]

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

[Andras Becsi]

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

[Andras Becsi]

Created attachment 88990 [details]
proposed fix

Simon or Jocelyn could you confirm whether this is the correct behaviour of the API?

[Andreas Kling]

Comment on attachment 88990 [details]
proposed fix

This is a progression IMO, so r=me. :)

[WebKit Commit Bot]

Comment on attachment 88990 [details]
proposed fix

Clearing flags on attachment: 88990

Committed r83440: <http://trac.webkit.org/changeset/83440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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?

[Ademar Reis]

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

[Ian Henderson]

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

[Ademar Reis]

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

[Ademar Reis]

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

[Ademar Reis]

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

[Ademar Reis]

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

[Caio Marcelo de Oliveira Filho]

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

[Ademar Reis]

(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

[Ademar Reis]

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.