Bug 58032

Summary: [Qt] REGRESSION(83122): tst_QWebElement::style() fails
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Ademar Reis <ademar>
Status: RESOLVED FIXED    
Severity: Major CC: abecsi, ademar, cmarcelo, commit-queue, hausmann, ian, jturcotte, koivisto
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 38654    
Attachments:
Description Flags
proposed fix
none
second fix (basically reverts the previous change) none

Description Csaba Osztrogonác 2011-04-07 05:47:31 PDT
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)]
Comment 1 Andras Becsi 2011-04-07 08:53:30 PDT
(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.
Comment 2 Andras Becsi 2011-04-11 04:57:58 PDT
(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.
Comment 3 Andras Becsi 2011-04-11 05:02:05 PDT
Created attachment 88990 [details]
proposed fix

Simon or Jocelyn could you confirm whether this is the correct behaviour of the API?
Comment 4 Andreas Kling 2011-04-11 06:17:30 PDT
Comment on attachment 88990 [details]
proposed fix

This is a progression IMO, so r=me. :)
Comment 5 WebKit Commit Bot 2011-04-11 06:45:10 PDT
Comment on attachment 88990 [details]
proposed fix

Clearing flags on attachment: 88990

Committed r83440: <http://trac.webkit.org/changeset/83440>
Comment 6 WebKit Commit Bot 2011-04-11 06:45:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Csaba Osztrogonác 2011-04-18 16:21:16 PDT
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?
Comment 8 Ademar Reis 2011-05-03 15:35:36 PDT
(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?
Comment 9 Ian Henderson 2011-05-03 15:38:52 PDT
Is this the same bug as <https://bugs.webkit.org/show_bug.cgi?id=60007>?
Comment 10 Ademar Reis 2011-05-04 07:18:05 PDT
(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.
Comment 11 Ademar Reis 2011-05-06 07:38:22 PDT
As described in my previous comment, the regression itself was fixed. To describe the real problem I've opened bug 60372.
Comment 12 Ademar Reis 2011-05-06 11:48:08 PDT
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
Comment 13 Ademar Reis 2011-05-06 11:50:53 PDT
Created attachment 92617 [details]
second fix (basically reverts the previous change)
Comment 14 Caio Marcelo de Oliveira Filho 2011-05-06 12:09:03 PDT
(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?
Comment 15 Ademar Reis 2011-05-06 12:20:26 PDT
(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
Comment 16 Ademar Reis 2011-05-06 13:51:30 PDT
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.