Bug 60372 - [Qt] tst_QWebElement::style() fails because QWebElement::InlineStyle now works as expected
Summary: [Qt] tst_QWebElement::style() fails because QWebElement::InlineStyle now work...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jocelyn Turcotte
URL:
Keywords: Qt
Depends on:
Blocks: 38654
  Show dependency treegraph
 
Reported: 2011-05-06 07:34 PDT by Ademar Reis
Modified: 2012-11-12 07:31 PST (History)
7 users (show)

See Also:


Attachments
mark failing test case as expected fail (1.42 KB, patch)
2011-05-20 06:53 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (2.00 KB, patch)
2012-11-09 08:35 PST, Jocelyn Turcotte
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ademar Reis 2011-05-06 07:34:57 PDT
The QWebElement documentation says that about QWebElement::InlineStyle: "Return the property value as it is defined in the element, without respecting style inheritance and other CSS rules".

In our QWebElement test, we do the following:

"""
    ...
    p.setStyleProperty("color", "green !important");
    QCOMPARE(p.styleProperty("color", QWebElement::InlineStyle), QLatin1String("green"));
    QCOMPARE(p.styleProperty("color", QWebElement::CascadedStyle), QLatin1String("green"));

    p.setStyleProperty("color", "blue");
    QCOMPARE(p.styleProperty("color", QWebElement::InlineStyle), QLatin1String("blue"));
    ...
"""

But the test fails:

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

When this was first implemented, the test expectation was wrong and thus the test was passing. Then there was a change in the CSS code that introduced a regression that curiously made our implementation actually work (and the test fail). The test was fixed and right after so was the regression, so now we have a broken test (as it should be).

See Bug 58032 for the whole story.
Comment 1 Ademar Reis 2011-05-06 11:43:45 PDT
From WebCore::CSSMutableStyleDeclaration::setProperty():

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

In the testing 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.

In summary, the implementation is right and the test is wrong, so it's just a matter or reverting the previous "fix".
Comment 2 Ademar Reis 2011-05-06 13:52:15 PDT
Reopening, please ignore my previous round of comments. :-P

I'll make sure my patch includes better documentation to avoid more confusion.
Comment 3 Csaba Osztrogonác 2011-05-20 06:53:01 PDT
Created attachment 94212 [details]
mark failing test case as expected fail
Comment 4 Ademar Reis 2011-05-20 06:56:08 PDT
Comment on attachment 94212 [details]
mark failing test case as expected fail

LGTM. Fixing this will take some more time (time that I don't have at the moment) :(
Comment 5 WebKit Commit Bot 2011-05-20 07:29:28 PDT
Comment on attachment 94212 [details]
mark failing test case as expected fail

Clearing flags on attachment: 94212

Committed r86953: <http://trac.webkit.org/changeset/86953>
Comment 6 WebKit Commit Bot 2011-05-20 07:29:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Csaba Osztrogonác 2011-05-20 07:30:34 PDT
Reopen to fix it.
Comment 8 Ademar Reis 2011-06-01 12:32:57 PDT
Revision r86953 cherry-picked into qtwebkit-2.2 with commit b6ea329 <http://gitorious.org/webkit/qtwebkit/commit/b6ea329>
Comment 9 Jocelyn Turcotte 2012-11-09 08:32:22 PST
Ademar was right in comment #1, a non-important inline style shouldn't be overwritten by a non-important one. So InlineStyle doesn't apply inheritance, but does consider the importance.
Comment 10 Jocelyn Turcotte 2012-11-09 08:35:16 PST
Created attachment 173313 [details]
Patch

Unmark the failure and fix the expected value.
Comment 11 Jocelyn Turcotte 2012-11-12 07:31:15 PST
Committed r134233: <http://trac.webkit.org/changeset/134233>