456147 - Fix color and layering of text-decoration:line-through in XUL textboxes.

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: fix [Checkin: Comment 2]

There is a typo in nsTextBoxFrame.cpp which causes it to draw text-decoration:line-through styling with the color appropriate for text-decoration:underline on that frame.  I imagine nobody has ever noticed, as you have to go out of your way to get different colors for the two lines; I noticed it while doing a warnings sweep.

Comment 1

[Zack Weinberg (:zwol)]

this is simple enough that I should think it doesn't need sr?

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 339535 [details] [diff] [review]
fix
[Checkin: Comment 2]

http://hg.mozilla.org/mozilla-central/rev/29ca9b9f5d3c

Comment 3

[[e-mail]]

Comment on attachment 339535 [details] [diff] [review]
fix
[Checkin: Comment 2]

>+   style="padding: 8px; background-color: white;">
Shouldn't you set these styles on the HTML version too, in case the defaults change?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Linux mozilla-central qm-centos5-03 dep unit test on 2008/09/20 11:46:33
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2slave/trunk_linux-3/build/layout/reftests/bugs/colored-strike.xul |

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Failed also elsewhere. I backed this out.

Comment 6

[Zack Weinberg (:zwol)]

I've been having problems with the reftests.  Could someone who sees this test and only this test fail (with the patch) please attach a reftest logfile to the bug?

Comment 7

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

You should be able to find plenty in the tinderbox logs.

Comment 8

[Neil Deakin]

Yes, I was going to ask about this but assumed it was tested. I would have thought that the padding/margins/borders and other spacing would have been different between blocks in the html version and the xul boxes, and also between platforms.

Comment 9

[Zack Weinberg (:zwol)]

(In reply to comment #8)
> I would have thought that the padding/margins/borders and other spacing
> would have been different between blocks in the html version and the
> xul boxes, and also between platforms.

This doesn't *seem* to be a problem, but the next revision of the test will specify margin and padding explicitly for every element in both files, just to be sure.  I suppose there's also a potential problem with line spacing versus spacing between elements in a <vbox>; but if <vbox> doesn't introduce spacing between elements unless explicitly told to, I think it's going to wind up being the font-specified line spacing in both cases.

(In reply to comment #4)
> Linux mozilla-central qm-centos5-03 dep unit test on 2008/09/20 11:46:33
> REFTEST TEST-UNEXPECTED-FAIL |
> file:///builds/moz2slave/trunk_linux-3/build/layout/reftests/bugs/colored-strike.xul

This turns out to be because HTML draws the strikeout line on top of the text, XUL draws it underneath.  Is that something that should be harmonized, and if so, which way?  I can patch for that, or I can change the strikeout line to black, which should make the difference invisible.

Comment 10

[Neil Deakin]

> This turns out to be because HTML draws the strikeout line on top of the text,
> XUL draws it underneath. 

I'd think that drawing it underneath is a bug.

Comment 11

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

Strikeout should be on top of the text per http://www.w3.org/TR/CSS21/zindex.html point 7.2.1.4.1.1.

Comment 12

[Zack Weinberg (:zwol)]

Attachment: revised patch

Here's a revised patch which changes strikethrough in XUL textboxes to be drawn on top of the text, and adds explicit specification of all the relevant CSS properties in both the XUL and HTML.  I am still having weird problems with the reftests, but this test does pass.

It's kind of a shame that XUL has its own frames...

Comment 13

[Zack Weinberg (:zwol)]

Attachment: rev 1a: fix test case naming convention and rediff

This slight revision fixes up the test case to use the standard naming convention for reftests/bugs/ (i.e. the file name stem is the bug number).  Also, reftest.list needed rediffing because some other patches landed.

carrying enn's r+ forward, still needs sr.

Comment 14

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

Comment on attachment 340611 [details] [diff] [review]
rev 1a: fix test case naming convention and rediff

sr=dbaron

Comment 15

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/5d283663b6d6

Comment 16

[Markus Stange [:mstange]]

The reftest was failing on all platforms because of different line heights. I changed the test to set all heights to 30px:
http://hg.mozilla.org/mozilla-central/rev/4def37ea72e4

Comment 17

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

Does changing the inline to a block substantively break the test?

Comment 18

[Markus Stange [:mstange]]

I don't think so - the updated test still tests this bug; i.e. without the patch in this bug, the line-through color is wrong.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Still failing on Mac -- the test content is drawn 1px higher than the reference content.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Marked test as failing on mac; mstange is trying to find a regression range.