Closed Bug 456147 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: zwol, Assigned: zwol)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch fix [Checkin: Comment 2] (obsolete) — Splinter Review
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.
Attachment #339535 - Flags: review?(enndeakin)
Attachment #339535 - Flags: review?(enndeakin) → review+
this is simple enough that I should think it doesn't need sr?
Keywords: checkin-needed
Comment on attachment 339535 [details] [diff] [review]
fix
[Checkin: Comment 2]

http://hg.mozilla.org/mozilla-central/rev/29ca9b9f5d3c
Attachment #339535 - Attachment description: fix → fix [Checkin: Comment 2]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
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?
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 |
Failed also elsewhere. I backed this out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
You should be able to find plenty in the tinderbox logs.
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.
(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.
> 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.
Strikeout should be on top of the text per http://www.w3.org/TR/CSS21/zindex.html point 7.2.1.4.1.1.
Attached patch revised patch (obsolete) — Splinter Review
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...
Attachment #339535 - Attachment is obsolete: true
Attachment #339700 - Flags: superreview?(enndeakin)
Attachment #339700 - Flags: review?(enndeakin)
Attachment #339700 - Flags: superreview?(enndeakin)
Attachment #339700 - Flags: superreview?(dbaron)
Attachment #339700 - Flags: review?(enndeakin)
Attachment #339700 - Flags: review+
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.
Attachment #339700 - Attachment is obsolete: true
Attachment #340611 - Flags: superreview?(dbaron)
Attachment #340611 - Flags: review+
Attachment #339700 - Flags: superreview?(dbaron)
Summary: nsTextBoxFrame (XUL <label>, <description>, <text>) uses underline color for line-through → Fix color and layering of text-decoration:line-through in XUL textboxes.
Comment on attachment 340611 [details] [diff] [review]
rev 1a: fix test case naming convention and rediff

sr=dbaron
Attachment #340611 - Flags: superreview?(dbaron) → superreview+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5d283663b6d6
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
Depends on: 458047
Does changing the inline to a block substantively break the test?
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.
Still failing on Mac -- the test content is drawn 1px higher than the reference content.
Marked test as failing on mac; mstange is trying to find a regression range.
You need to log in before you can comment on or make changes to this bug.