338374 - Mac doesn't support non-solid line drawing

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Mac, defect)

Description

[Brett Wilson]

The Mac SetLineStyle code is here:
http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsRenderingContextMac.cpp#773

It just saves the line style away. This variable seems totally unused except for the getter/setter, and the getter is never called by the Mac graphics code.

For reference, the GTK version is here:
http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsRenderingContextGTK.cpp#716

You might be able to use PenPat:
http://developer.apple.com/documentation/Carbon/reference/QuickDraw_Ref/Reference/reference.html#//apple_ref/c/func/PenPat

This affects inline spellchecking: the misspelling underlines are solid and look bad.

Comment 1

[Josh Aas]

Attachment: fix v1.0

Comment 2

[Brett Wilson]

Talking to Mark this morning, he said that we should do this call right before drawing the line (in DrawLine) and reset it immediately afterwards because the PenPat affects more than just lines.

Comment 3

[Josh Aas]

Attachment: fix v2.0 (alternative)

This is alternative patch. Instead of doing anything with QD when the line style is set, it sets QD to the appropriate pen pattern before drawing lines and sets QD back to the 'black' pen pattern after drawing. This way the pen pattern can only affect the line drawing operation.

Comment 4

[Joe Hughes]

Attachment: Screenshot of new spell-checking underline with patch #2

Patch #2 doesn't seem quite right--to match the Windows & Linux (branch) rendering, the "dotted" style should be one pixel on, one pixel off, but it looks like the results are two pixels gray, two pixels on.  (This is on an Intel iMac running 10.4.6.)

Comment 5

[Mark Mentovai]

Comment on attachment 222518 [details] [diff] [review]
fix v1.0

v2 is better

Comment 6

[Mark Mentovai]

Comment on attachment 222523 [details] [diff] [review]
fix v2.0 (alternative)

I'd prefer it if you called GetPenState to get the current penpat and restore it when you're done drawing your lines.

Comment 7

[Mark Mentovai]

(In reply to comment #4)
> Patch #2 doesn't seem quite right--to match the Windows & Linux (branch)
> rendering, the "dotted" style should be one pixel on, one pixel off, but it
> looks like the results are two pixels gray, two pixels on.

The gray is the background color, you can change it with ::RGBBackColor.

Comment 8

[Brett Wilson]

We really want the non-dashes to be transparent. It that possible?

Comment 9

[Josh Aas]

Attachment: fix v2.1

This patch makes dotted lines have 1-pixel dots. It also does save/restore of RGB back color and pen state.

I don't know of a way to draw dotted lines transparently short of completely rewriting the way we draw lines, so I just made the back color on dotted/dashed lines white all of the time. I'm well aware of the fact that such an approach sucks, and I'm happy to hear ideas.

Comment 10

[Joe Hughes]

Attachment: Screenshot of patch 2.1

Looks more correct now--is there no way to sample the background color to make it "transparent"?  I'm concerned about textboxes with odd background colors.

Comment 11

[Josh Aas]

What you're saying about sampling background colors is what I mean by rewriting the way we draw lines. That is probably possible but a pain in the butt.

We may be able to do something with SetPortPenPixPat, set up a 1-bit mask for the graphics port and get transparency that way. With 1-pixel dotted lines the potential for problems here is that our fill color pixels will overlap with the holes in the mask.

Another option which might be somewhat better is to sample the color at the start point of the line and set that to the background color for the whole line. The problem then is that if the line passes through an area with different background colors we're in trouble. Or if the start point for the line is not the right color, say if the start is on the hanging part of a letter in a white-background text box.

Comment 12

[Josh Aas]

I tried sampling the color at the start point of the line and another big problem is what happens if we paint twice. The first time it works fine - we sample a white pixel and that becomes the spacer color. Problem is, the second time we sample a red pixel because we just drew the line there and then we draw a solid red line because the spacer is red. It is easy to see this in the spellchecking example - if you misspell a word it initially paints the line correctly, but when you type the next word it repaints the line to solid red. Not that I had high hopes for this approach anyway...

A somewhat more successful version of this hack is sampling the pixel to the right of the origin of the line. This pixel is always an off-pixel so we don't have the double-draw sampling problem. However, this won't work as well for vertical lines. We could try to determine if a line is mostly vertical or mostly horizontal and sample up or to the right based on the results. We could sample a few pixels off for the dashed line. If anyone is interested in trying this, I can post a patch.

Comment 13

[Josh Aas]

Attachment: "smart" sampling fix, v3.0

This does "smart sampling" as outlined in the second part of comment 12, also including logic for taking line direction into account. It works well for the spellchecking case, and should work well in most cases that might actually happen.

Note: The smart sampling logic is only implemented for dotted lines, not dashed lines (spellchecking uses dotted). I could implement it for dashed lines if people like this approach.

Comment 14

[Josh Aas]

Mark suggested using the transparent mode for CopyBits, and now that some more about QuickDraw is clear to me I understand how that might work. I'll write a patch for that approach soon.

Comment 15

[Josh Aas]

Attachment: fix v4.0

It looks like this is going to work with CopyBits. This way we can have actual transparent pixels between the dots in the line. This patch works great for the spellchecking example with a red line over a white textbox.

What worries me about this patch is what happens when the line is black or white, or the background it is over is black. Those cases I have not tried yet, everything else should be fine.

Brett - can you point me to the code where I can change the color of the spellchecking underline line? If this patch checks out in those cases I'll request review.

Comment 16

[Brett Wilson]

The patch on bug 338209 changes the wavy underline code. The code on trunk is here: http://lxr.mozilla.org/seamonkey/source/layout/generic/nsTextFrame.cpp#2585
On branch there is similar code.

Comment 17

[Josh Aas]

Attachment: test html

This is a good test case for my patches.

Comment 18

[Josh Aas]

Attachment: fix v4.2

This patch should do everything correctly for regular lines, dotted lines, and dashed lines, including transparency. It works in all cases for me, different background colors and different lines colors.

I was messing around with optimizing this a bit by not making the offscreen gworld as big as the one it is copying into, but I decided against it. I doubt it is much of a penalty, certainly not as much as we use it. Another issue with that is when you're trying to make a gworld to fit just a 1-pixel-tall horizontal line (for example), the gworld rejects the bounding rect for that. So if your line goes from (10,22) -> (100,22), the gworld will reject the bounding rect for that line and just become really really big. You'd have to make the gworld at least one pixel bigger on all sides than whatever you are bounding, and that just makes this probably unnecessary optimization messier. The code in this patch is pretty clean and easy to understand.

Comment 19

[Mark Mentovai]

Comment on attachment 222999 [details] [diff] [review]
fix v4.2

Looks good - all comments were given/accounted for on irc, just add a comment at the top of each block saying that the code is duplicated and why it would have been heinous to move it off into its own proc.  It would have been nice to do smaller copies, but oh well.

Comment 20

[Joe Hughes]

Attachment: Screenshot of 4.2--looks good!

Comment 21

[Brett Wilson]

Comment on attachment 222999 [details] [diff] [review]
fix v4.2

I don't think I'm allowed to give SR for this module. Looks good to me, though.

Comment 22

[Mark Mentovai]

(In reply to comment #20)
> Created an attachment (id=223074) [edit]
> Screenshot of 4.2--looks good!

The question is, does the suggest feature offer "base" as an alternative for "sfo?"

Comment 23

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 222999 [details] [diff] [review]
fix v4.2

sr=pink

Comment 24

[Josh Aas]

checked in on trunk

Comment 25

[Josh Aas]

This is important for spellchecking to draw correctly so targetting 1.8.1a3

Comment 26

[Brett Wilson]

Definitely too late for a3. B1 should be fine, though.

Comment 27

[Josh Aas]

Attachment: fix v5.0

I didn't know about the transparent pen mode and how that interacts with patterns until Mark mentioned it might work today. Turns out it does. This should fix the Tp problems.

Comment 28

[Mark Mentovai]

Comment on attachment 223381 [details] [diff] [review]
fix v5.0

r=me, but I want you to make two changes to both methods:

Call ::RGBBackColor with the result of ::InvertColor of the foreground color, and restore ::RGBBackColor to the saved initial state obtained from ::GetBackColor.

and

Add this check as the first part of each method:

  if (mLineStyle == nsLineStyle_kNone)
    return NS_OK;

Those are straightforward enough that you can make them on checkin and I'll look at it later if you don't feel like posting a new patch now.

Comment 29

[Josh Aas]

Attachment: fix v5.1

implement mark's suggestions

Comment 30

[Mark Mentovai]

Comment on attachment 223389 [details] [diff] [review]
fix v5.1

I wanted you to invert the fore color, not the back color.

Comment 31

[Josh Aas]

my bad, fixed on trunk

Comment 32

[Mark Mentovai]

Trunk fix looks good.

Comment 33

[Josh Aas]

Attachment: 1.8.1 branch fix v5.2

Comment 34

[Mark Mentovai]

Comment on attachment 223452 [details] [diff] [review]
1.8.1 branch fix v5.2

r++

Comment 35

[Mike Connor [:mconnor]]

moving on out, will get this for beta1

Comment 36

[Josh Aas]

landed on 1.8.1 branch