diff options
author | Volker Hilsheimer <volker.hilsheimer@qt.io> | 2021-10-11 13:03:44 +0200 |
---|---|---|
committer | Qt Cherry-pick Bot <cherrypick_bot@qt-project.org> | 2021-10-12 16:37:42 +0000 |
commit | 9a733dabc8c6e75cc2b1668a58671fd148728acc (patch) | |
tree | 94d9833f550a892e29bb09b85d99f4f0a6893806 | |
parent | 8b65905e4b8c12040373cc36ec29dbe2861c24a9 (diff) |
Fix caching of parsed border color values in CSS parser
When parsing CSS, a border-color value is parsed as four brushes, as css
allows assigning up to four values, one for each side.
When applying the CSS to the HTML, we accessed it as a color value,
which overwrote the parsed value with a QColor. So while we had a valid
parsed value (and didn't re-parse), the code accessing that value still
expected it to be a list, and thus failed to retrieve the data.
There are several ways to fix that, but the cleanest way without
introducing any performance penalty from repeatedly parsing (and in fact
removing a parse of the string into a color) is to enable colorValue to
interpret an already parsed value that is a list without overwriting the
parsed value again. To avoid similar issues in the future, add assert
that the parsed value has the right type in brushValues.
As a drive-by, speed things up further by making use of qMetaTypeId
being constexpr, which allows for it to be used in a switch statement.
Add a test case.
Fixes: QTBUG-96603
Change-Id: Icdbff874daedc91bff497cd0cd1d99e4c713217c
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
(cherry picked from commit 8513bcd90cc3900f9c32e59e0823f621ea6eea1d)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
-rw-r--r-- | src/gui/text/qcssparser.cpp | 13 | ||||
-rw-r--r-- | tests/auto/gui/text/qtextdocument/tst_qtextdocument.cpp | 124 |
2 files changed, 135 insertions, 2 deletions
diff --git a/src/gui/text/qcssparser.cpp b/src/gui/text/qcssparser.cpp index 574436d6f6..da3de4d651 100644 --- a/src/gui/text/qcssparser.cpp +++ b/src/gui/text/qcssparser.cpp @@ -1460,10 +1460,18 @@ QColor Declaration::colorValue(const QPalette &pal) const return QColor(); if (d->parsed.isValid()) { - if (d->parsed.userType() == QMetaType::QColor) + switch (d->parsed.typeId()) { + case qMetaTypeId<QColor>(): return qvariant_cast<QColor>(d->parsed); - if (d->parsed.userType() == QMetaType::Int) + case qMetaTypeId<int>(): return pal.color((QPalette::ColorRole)(d->parsed.toInt())); + case qMetaTypeId<QList<QVariant>>(): + if (d->parsed.toList().size() == 1) { + const auto &value = d->parsed.toList().at(0); + return qvariant_cast<QColor>(value); + } + break; + } } ColorData color = parseColorValue(d->values.at(0)); @@ -1507,6 +1515,7 @@ void Declaration::brushValues(QBrush *c, const QPalette &pal) const int i = 0; if (d->parsed.isValid()) { needParse = 0; + Q_ASSERT(d->parsed.metaType() == QMetaType::fromType<QList<QVariant>>()); QList<QVariant> v = d->parsed.toList(); for (i = 0; i < qMin(v.count(), 4); i++) { if (v.at(i).userType() == QMetaType::QBrush) { diff --git a/tests/auto/gui/text/qtextdocument/tst_qtextdocument.cpp b/tests/auto/gui/text/qtextdocument/tst_qtextdocument.cpp index 1a644d1bdd..74c8bfd260 100644 --- a/tests/auto/gui/text/qtextdocument/tst_qtextdocument.cpp +++ b/tests/auto/gui/text/qtextdocument/tst_qtextdocument.cpp @@ -124,7 +124,10 @@ private slots: void clonePreservesIndentWidth(); void clonePreservesFormatsWhenEmpty(); void blockCount(); + void defaultStyleSheet(); + void defaultTableStyle_data(); + void defaultTableStyle(); void resolvedFontInEmptyFormat(); @@ -2648,6 +2651,127 @@ void tst_QTextDocument::defaultStyleSheet() QVERIFY(fmt.background().color() != QColor("green")); } +void tst_QTextDocument::defaultTableStyle_data() +{ + QTest::addColumn<QString>("css"); + QTest::addColumn<QString>("html"); + QTest::addColumn<QList<QBrush>>("borderBrushes"); + + const QString htmlHeader(R"( + <tr> + <th>1</th> <th>2</th> + </tr> + )"); + + const QString htmlCells(R"( + <tr> + <td>A</td> <td>B</td> + </tr> + )"); + + const QString cssEachSide = R"({ + border-top-color: red; + border-bottom-color: blue; + border-left-color: green; + border-right-color: yellow; + })"; + const QString cssOneColor = R"({ border-color: red; })"; + const QString cssFourColors = R"({ border-color: red blue green yellow; })"; + + QTest::addRow("td, each side") << QString("td ") + cssEachSide + << htmlCells + << QList<QBrush>{Qt::red, Qt::blue, QColor("green"), Qt::yellow}; + + QTest::addRow("th, each side") << QString("th ") + cssEachSide + << htmlHeader + << QList<QBrush>{Qt::red, Qt::blue, QColor("green"), Qt::yellow}; + + QTest::addRow("th+td, each side") << QString("th %1 td %1").arg(cssEachSide) + << htmlHeader + htmlCells + << QList<QBrush>{Qt::red, Qt::blue, QColor("green"), Qt::yellow}; + + QTest::addRow("td, one color") << QString("td ") + cssOneColor + << htmlCells + << QList<QBrush>{Qt::red, Qt::red, Qt::red, Qt::red}; + + QTest::addRow("th, one color") << QString("th ") + cssOneColor + << htmlHeader + << QList<QBrush>{Qt::red, Qt::red, Qt::red, Qt::red}; + + QTest::addRow("th+td, one color") << QString("th %1 td %1").arg(cssOneColor) + << htmlHeader + htmlCells + << QList<QBrush>{Qt::red, Qt::red, Qt::red, Qt::red}; + + // css order is top, right, bottom, left + QTest::addRow("td, four colors") << QString("td ") + cssFourColors + << htmlCells + << QList<QBrush>{Qt::red, QColor("green"), Qt::yellow, Qt::blue}; + +} + +void tst_QTextDocument::defaultTableStyle() +{ + QFETCH(QString, css); + QFETCH(QString, html); + QFETCH(QList<QBrush>, borderBrushes); + + CREATE_DOC_AND_CURSOR(); + doc.setDefaultStyleSheet(css); + doc.setHtml(QString("<html><body><table>%1</table></body></html>").arg(html)); + + const QTextFrame *frame = doc.rootFrame(); + const QTextTable *table = nullptr; + for (auto it = frame->begin(); !table && !it.atEnd(); ++it) + table = qobject_cast<const QTextTable*>(it.currentFrame()); + QVERIFY(table); + + const QList<QTextFormat::Property> brushProperties = { + QTextFormat::TableCellTopBorderBrush, + QTextFormat::TableCellBottomBorderBrush, + QTextFormat::TableCellLeftBorderBrush, + QTextFormat::TableCellRightBorderBrush + }; + + for (int row = 0; row < table->rows(); ++row) { + for (int column = 0; column < table->columns(); ++column) { + auto cellDetails = qScopeGuard([&]{ + qWarning("Failure was in cell %d/%d", row, column); + }); + const QTextTableCell cell = table->cellAt(row, column); + const QTextTableCellFormat cellFormat = cell.format().toTableCellFormat(); + QList<QBrush> brushes; + for (const auto side : brushProperties) { + QVariant sideProperty = cellFormat.property(side); + QVERIFY(sideProperty.isValid()); + QVERIFY(sideProperty.typeId() == qMetaTypeId<QBrush>()); + brushes << sideProperty.value<QBrush>(); + } + auto errorDetails = qScopeGuard([&]{ + if (brushes.count() != borderBrushes.count()) { + qWarning("Different count: %lld vs %lld", brushes.count(), borderBrushes.count()); + return; + } + for (int i = 0; i < brushes.count(); ++i) { + QString side; + QDebug(&side) << QTextFormat::Property(QTextFormat::TableCellTopBorderBrush + i); + QString actual; + QDebug(&actual) << brushes.at(i); + QString expected; + QDebug(&expected) << borderBrushes.at(i); + if (expected != actual) { + qWarning("\n %s:\n\tActual: %s\n\tExpected:%s", qPrintable(side), + qPrintable(actual), qPrintable(expected)); + } + } + }); + QVERIFY2(borderBrushes == brushes, // QCOMPARE doesn't generate helpful output anyway + qPrintable(QString("for cell %1/%2").arg(row).arg(column))); + errorDetails.dismiss(); + cellDetails.dismiss(); + } + } +} + void tst_QTextDocument::maximumBlockCount() { QCOMPARE(doc->maximumBlockCount(), 0); |