Bug 94179

Summary:

[Qt] -webkit-text-stroke values being passed through a non-inherited chain

Product:

WebKit

Reporter:

Bruno Abinader (history only) <bruno.abinader>

Component:

Layout and Rendering

Assignee:

Caio Marcelo de Oliveira Filho <cmarcelo>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

benjamin, buildbot, cmarcelo, dglazkov, eric, igor.oliveira, jesus, martin.leutelt, menard, noam, rafael.lobo, reed, rniwa, senorblanco, webkit.review.bot

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Example to test the issue	none
Patch	none
Archive of layout-test-results from gce-cr-linux-01	none
Patch	none
Patch	webkit.review.bot: commit-queue-

Bruno Abinader (history only)

Reported 2012-08-15 20:27:04 PDT

Created attachment 158700 [details] Example to test the issue While implementing repaint tests for bug 91638, I've noticed that the '-webkit-text-stroke' style property, when used on an object, affects the others not directly linked to this object (ie. through inheritance). See the example below: ... <body> red fill, blue stroke text with underline This is a very long text that should have its text stroke values reset to default </body> ... The -webkit-text-stroke values from the span inside line1 are erroneously used on line2. On Minibrowser, the text stroke values are used until a certain length of the text ("reset" word), and inside QtTestBrowser it goes all the way. This behavior is certainly wrong and indicates that the text stroke width/color are not reset between each object rendering. Can someone please also confirm this? I haven't tested it yet, but this issue might appear on other platforms as well.

Attachments
Example to test the issue (333 bytes, text/html) 2012-08-15 20:27 PDT, Bruno Abinader (history only)	no flags	Details
Patch (15.05 KB, patch) 2012-08-20 16:00 PDT, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-01 (321.02 KB, application/zip) 2012-08-20 17:09 PDT, WebKit Review Bot	no flags	Details
Patch (50.50 KB, patch) 2012-09-15 06:30 PDT, Caio Marcelo de Oliveira Filho	no flags	Details Formatted Diff Diff
Patch (50.56 KB, patch) 2012-09-15 06:50 PDT, Caio Marcelo de Oliveira Filho	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Martin Leutelt

Comment 1 2012-08-16 06:05:48 PDT

Verified in both QtTestBrowser and Minibrowser with webkit built from 8192d0. The second line looks like bold text.

Caio Marcelo de Oliveira Filho

Comment 2 2012-08-17 10:09:50 PDT

Same problem here with MiniBrowser and QtTestBrowser, the stroke property "leaks" to line2.

Jesus Sanchez-Palencia

Comment 3 2012-08-20 08:33:03 PDT

Is this Qt specific?

Caio Marcelo de Oliveira Filho

Comment 4 2012-08-20 16:00:57 PDT

Created attachment 159550 [details] Patch

Build Bot

Comment 5 2012-08-20 16:58:37 PDT

Comment on attachment 159550 [details] Patch Attachment 159550 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13544532

Benjamin Poulain

Comment 6 2012-08-20 16:58:48 PDT

Comment on attachment 159550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159550&action=review > Source/WebCore/ChangeLog:17 > + Other ports don't have this issue because they check both text stroke flag and > + the stroke thickness. Their could could be simplified in the future. Typo: "Their could" > Source/WebCore/rendering/InlineTextBox.cpp:325 > +void setTextModeStrokeEnabled(GraphicsContext* context, bool enabled) static? I am surprised the compiler does not bitch about that. > Source/WebCore/rendering/InlineTextBox.cpp:334 > + TextDrawingModeFlags newMode = context->textDrawingMode(); > + if (enabled) > + newMode |= TextModeStroke; > + else > + newMode &= ~TextModeStroke; > + > + if (context->textDrawingMode() == newMode) > + return; This is a big change of behavior. Previously, any update of TextModeStroke in InlineTextBox::paint() would cascade. I am surprised you only need one test.

WebKit Review Bot

Comment 7 2012-08-20 17:08:55 PDT

Comment on attachment 159550 [details] Patch Attachment 159550 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13542533 New failing tests: fast/text/stroke-not-leaking-to-sibling.html

WebKit Review Bot

Comment 8 2012-08-20 17:09:00 PDT

Created attachment 159565 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Caio Marcelo de Oliveira Filho

Comment 9 2012-09-15 06:30:01 PDT

Created attachment 164285 [details] Patch

Caio Marcelo de Oliveira Filho

Comment 10 2012-09-15 06:50:15 PDT

Created attachment 164286 [details] Patch

Caio Marcelo de Oliveira Filho

Comment 11 2012-09-15 07:06:11 PDT

Comment on attachment 159550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159550&action=review >> Source/WebCore/rendering/InlineTextBox.cpp:325 >> +void setTextModeStrokeEnabled(GraphicsContext* context, bool enabled) > > static? > I am surprised the compiler does not bitch about that. Fixed. >> Source/WebCore/rendering/InlineTextBox.cpp:334 >> + return; > > This is a big change of behavior. Previously, any update of TextModeStroke in InlineTextBox::paint() would cascade. I am surprised you only need one test. In this new patch I'm also changing Chromium to rely on the flag only (not on the thickness) to verify whether we are causing regressions. All the calls to updateGraphicsContext() inside InlineTextBox::paint() are guarded by a GraphicsContextStateSaver, that should restore the text drawing mode, except for the code for text decoration paintings. I added new a test using decoration to exercise this code a bit. After the decoration we don't draw text, we may draw markings (like spelling markers) and then composition background. In both cases we don't seem to use text drawing flag. I don't think callers of InlineTextBox::paint() expect some specific text drawing is set after calling it.

WebKit Review Bot

Comment 12 2012-09-15 09:51:15 PDT

Comment on attachment 164286 [details] Patch Attachment 164286 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13858557 New failing tests: fast/text/stroke-nesting.html fast/text/stroke-not-leaking-to-sibling.html

Caio Marcelo de Oliveira Filho

Comment 13 2012-10-10 10:11:43 PDT

Ping?

Build Bot

Comment 14 2013-01-16 09:08:24 PST

Comment on attachment 164286 [details] Patch Attachment 164286 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15905482 New failing tests: fast/text/stroke-nesting.html fast/text/stroke-not-leaking-to-sibling.html

Bruno Abinader (history only)

Comment 15 2013-04-18 07:01:51 PDT

Re-ping? :-) The proposed patch might require some rebase.

Anders Carlsson

Comment 16 2013-10-02 21:15:57 PDT

Comment on attachment 164286 [details] Patch Qt has been removed, clearing review flags.

Jocelyn Turcotte

Comment 17 2014-02-03 03:22:05 PST

=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.

Note You need to log in before you can comment on or make changes to this bug.