210330 - Underlined links appear as overstrikes

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Doug Lerner]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4) Gecko/20030612
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4) Gecko/20030612

At many sites, including the http://www.mozilla.org/start/ page, hotlinked text
with underlines render as overstrikes instead of underlines.


Reproducible: Always

Steps to Reproduce:
1.Go to the site. That is it.
2.
3.

Actual Results:  
As described above.

Expected Results:  
Displayed the underlined links with underlines rather than overstrikes.

Comment 1

[Doug Lerner]

Attachment: this screenshot demonstrates the overstrike rendering bug mentioned here

Comment 2

[Boris Zbarsky [:bzbarsky]]

Over to mac gfx -- sounds like a platform font issue.

Comment 3

[Simon Fraser [no longer active]]

What font is the page being rendered in? If you change the default font, does
the problem go away? What if you change the font size?

Comment 4

[David Lawhon]

The font used here appears to be "Arial." Using that font, the page (as it now
exists) WFM. However, the page is significantly different-looking now than when
this bug was originally submitted, due to a site redesign. I always use "Arial"
as my sans-serif font, though, and have never experienced this problem on any page.

I'm currently using Mozilla 1.8a4 (8/28/04 build) under MacOSX 10.3.5.

-- David Lawhon

Comment 5

[Uri Bernstein]

I'm seeing this issue when the language is set to "he" (Hebrew), and the default
font is used (regardless, it appears, of what the default font actually is).

This bug is very easy to spot on the Hebrew Wikipedia (http://he.wikipedia.org). 

Testcase and screenshot coming up.

Confirming bug.
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050510
Firefox/1.0+

Comment 6

[Uri Bernstein]

Attachment: testcase

Testcase with various combinations of language and font settings.

Comment 7

[Uri Bernstein]

Attachment: screenshot of testcase

Screenshot, with default font for Hebrew set to "Lucida Grande".

Comment 8

[Uri Bernstein]

Attachment: patch

My first-ever patch. Please be gentle!

The issue was that the LangGroup information wasn't passed to GetMetricsFor() -
which therefore used the default font associated with the default LangGroup.
Since the metrics of this font could be different than those of the default
font for the actual language used (Hebrew, in my example) - the underline ended
up in the wrong place.

I took the method for figuring out the LangGroup from
nsFrame::SetFontFromStyle(). I hope this is correct.

I guess this means this is not a Mac-specific bug. I sure hope I'm on the right
bug at all (it's remotely possible that the original bug was not the same
issue).

Comment 9

[Uri Bernstein]

Comment on attachment 183335 [details] [diff] [review]
patch

requesting review from bzbarsky. Once again - this is my first patch. Please
point out to me whatever I did wrong both in procedure and in code.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 183335 [details] [diff] [review]
patch

Looks reasonable, though you should be able to just call GetStyleVisibility()
(withou the GetStyleContext() part) on a frame.

Going to ask Simon for the actual review, though, just in case.  We seem to
have very few people actually passing the style lang group to GetMetricsFor();
perhaps we need more followup bugs?

Comment 11

[Uri Bernstein]

Attachment: patch v. 2

Patch with Boris' comment implemented. Carrying over flags (hope this is OK).

Comment 12

[Simon Montagu :smontagu]

(In reply to comment #10)
> We seem to
> have very few people actually passing the style lang group to GetMetricsFor();
> perhaps we need more followup bugs?

Yes, I think we do. I said something similar in bug 100762.

Comment 13

[Simon Montagu :smontagu]

Comment on attachment 183372 [details] [diff] [review]
patch v. 2

r=smontagu.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 183372 [details] [diff] [review]
patch v. 2

requesting 1.8b2 approval.  This is a very safe patch that should make us paint
text decorations better.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 183372 [details] [diff] [review]
patch v. 2

a=dbaron, although you really don't need the |visibility| variable at all...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed.