Bug 19447 – [Qt] First line of links not underlined correctly in some cases

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Jonathon Jongsma (jonner)]

Depending on several variables, there are times when QtWebKit doesn't draw the underline of links correctly.  If the link text is one line or less, that means it doesn't have an underline at all.  If it's more than one line, the first line does not have an underline, but all subsequent lines do.  I've produced a relatively minimal test-case, and the important cases seem to be 
a) the containing element has a border
b) the page is xhtml

I'll attach a test case and a screenshot showing the behavior.

[Jonathon Jongsma (jonner)]

Created attachment 21594 [details]
Test Case

[Jonathon Jongsma (jonner)]

Created attachment 21595 [details]
screenshot showing the first line of the link without an underline

[Jonathon Jongsma (jonner)]

A quick data point.  When context->drawLineForText() is called for the first line of text in this link, the context->m_common->state.strokeStyle is set to WebCore::NoStroke.  If I hack in the following line of code, the test case is displayed correctly on QtWebKit:

diff --git a/WebCore/rendering/InlineFlowBox.cpp b/WebCore/rendering/InlineFlowBox.cpp
index c3961a6..75e4129 100644
--- a/WebCore/rendering/InlineFlowBox.cpp
+++ b/WebCore/rendering/InlineFlowBox.cpp
@@ -962,6 +962,7 @@ void InlineFlowBox::paintTextDecorations(RenderObject::PaintInfo& paintInfo, int
             }
 
             if (paintUnderline) {
+                context->setStrokeStyle(WebCore::SolidStroke);
                 context->setStrokeColor(underline);
                 // Leave one pixel of white between the baseline and the underline.
                 context->drawLineForText(IntPoint(tx, ty + m_baseline + 1), w, isPrinting);


I'll need to investigate a bit more to figure out *why* this value is set to NoStroke on qt

[Jonathon Jongsma (jonner)]

I just discovered that this bug did affect the gtk port in the past, but it was worked around in this changeset: http://trac.webkit.org/changeset/26987

I could do the same thing in the Qt port, but it seems that it might be better to do this in WebCore instead of adding workarounds to all of the individual ports.

The root cause of this issue seems to be that borders are painted by drawing rectangles and filling them with no stroke.  So after the border is drawn, the graphics context stroke style is left as NoStroke, and the link underline will not be drawn correct if it's drawn with a stroke (as Qt and GTK ports do).  Ports that draw text underlines with a fill (e.g. mac) are not affected.  

Also worth noting is that if the border has a radius (e.g. -webkit-border-radius is non-zero), the graphics context will be saved and restored in RenderObject::paintBorder(), so the stroke style will not be left as NoStroke, so the link underline will work correctly.

So there are a couple of alternate ways to address this issue, I'm not sure which one is best.

[Jonathon Jongsma (jonner)]

Created attachment 21740 [details]
Option 1 of 3.  Add the workaround in Qt that exists in the Cairo port

[Jonathon Jongsma (jonner)]

Created attachment 21741 [details]
Option 2 of 3: save and restore the context in paintBorder

This makes is so that the Stroke Style is not NoStroke when we try to paint the link underline (not sure this is always true, however), therefore the workaround can be removed from the Cairo port

[Jonathon Jongsma (jonner)]

Created attachment 21742 [details]
Option 3 of 3: Explicitly set the stroke style before drawing text underline

In this case, we can also  remove the workaround from the cairo port.

[Simon Hausmann]

I personally like the third patch the best. I suggest to ask perhaps David Hyatt for a review of the third one.

[Simon Hausmann]

Comment on attachment 21742 [details]
Option 3 of 3: Explicitly set the stroke style before drawing text underline

This patch looks simple and safe. I hope Dave Hyatt can review this one :)

[Simon Hausmann]

Cleared review flag on the other two commits in favour of the third one

[Eric Seidel (no email)]

Comment on attachment 21742 [details]
Option 3 of 3: Explicitly set the stroke style before drawing text underline

dhyatt>
css3 is going to allow lines drawn for text
[3:36pm] <dhyatt>
to be other stroke styles
[3:36pm] <dhyatt>
so i don't like that
[3:37pm] <dhyatt>
that essentially hardcodes a solid stroke for this api
[3:37pm] <dhyatt>
it's supposed to use the current stroke style
[3:37pm] <dhyatt>
we should fix inlinetextbox
[3:38pm] <dhyatt>
not work around it

[Jonathon Jongsma (jonner)]

yes, that was my concern with options 1 and 3 as well.  Perhaps option 2 would be acceptable instead?

[Adam Treat]

This is fixed with r41152.  I didn't see this bug report, but did see the GTK+ version.  My fix was essentially the same as option 3, but applied to both inlinetext and inlineflow and reviewed by David Hyatt.