Bug 19447 - [Qt] First line of links not underlined correctly in some cases
Summary: [Qt] First line of links not underlined correctly in some cases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Jonathon Jongsma (jonner)
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2008-06-09 08:20 PDT by Jonathon Jongsma (jonner)
Modified: 2009-02-24 09:48 PST (History)
3 users (show)

See Also:


Attachments
Test Case (1.65 KB, text/html)
2008-06-09 08:23 PDT, Jonathon Jongsma (jonner)
no flags Details
screenshot showing the first line of the link without an underline (39.60 KB, image/png)
2008-06-09 08:28 PDT, Jonathon Jongsma (jonner)
no flags Details
Option 1 of 3. Add the workaround in Qt that exists in the Cairo port (752 bytes, patch)
2008-06-16 12:45 PDT, Jonathon Jongsma (jonner)
no flags Details | Formatted Diff | Diff
Option 2 of 3: save and restore the context in paintBorder (1.86 KB, patch)
2008-06-16 12:46 PDT, Jonathon Jongsma (jonner)
no flags Details | Formatted Diff | Diff
Option 3 of 3: Explicitly set the stroke style before drawing text underline (1.39 KB, patch)
2008-06-16 12:47 PDT, Jonathon Jongsma (jonner)
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathon Jongsma (jonner) 2008-06-09 08:20:14 PDT
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.
Comment 1 Jonathon Jongsma (jonner) 2008-06-09 08:23:00 PDT
Created attachment 21594 [details]
Test Case
Comment 2 Jonathon Jongsma (jonner) 2008-06-09 08:28:51 PDT
Created attachment 21595 [details]
screenshot showing the first line of the link without an underline
Comment 3 Jonathon Jongsma (jonner) 2008-06-13 12:32:27 PDT
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
Comment 4 Jonathon Jongsma (jonner) 2008-06-16 09:19:57 PDT
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.
Comment 5 Jonathon Jongsma (jonner) 2008-06-16 12:45:18 PDT
Created attachment 21740 [details]
Option 1 of 3.  Add the workaround in Qt that exists in the Cairo port
Comment 6 Jonathon Jongsma (jonner) 2008-06-16 12:46:50 PDT
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
Comment 7 Jonathon Jongsma (jonner) 2008-06-16 12:47:37 PDT
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.
Comment 8 Simon Hausmann 2008-06-20 03:15:05 PDT
I personally like the third patch the best. I suggest to ask perhaps David Hyatt for a review of the third one.
Comment 9 Simon Hausmann 2008-07-26 06:13:14 PDT
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 :)
Comment 10 Simon Hausmann 2008-07-26 06:15:39 PDT
Cleared review flag on the other two commits in favour of the third one
Comment 11 Eric Seidel (no email) 2008-08-01 15:43:34 PDT
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
Comment 12 Jonathon Jongsma (jonner) 2008-08-02 11:18:56 PDT
yes, that was my concern with options 1 and 3 as well.  Perhaps option 2 would be acceptable instead?
Comment 13 Adam Treat 2009-02-24 09:48:55 PST
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.