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

RESOLVED FIXED in mozilla1.9.1b1

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: zwol, Assigned: zwol)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9.1b1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 339535 [details] [diff] [review]
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.
Attachment #339535 - Flags: review?(enndeakin)
Attachment #339535 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 1

10 years ago
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
Last Resolved: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1

Comment 3

10 years ago
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

10 years ago
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

10 years ago
Failed also elsewhere. I backed this out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Comment 9

10 years ago
(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.
(Assignee)

Comment 12

10 years ago
Created attachment 339700 [details] [diff] [review]
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...
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+
(Assignee)

Comment 13

10 years ago
Created attachment 340611 [details] [diff] [review]
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.
Attachment #339700 - Attachment is obsolete: true
Attachment #340611 - Flags: superreview?(dbaron)
Attachment #340611 - Flags: review+
Attachment #339700 - Flags: superreview?(dbaron)
(Assignee)

Updated

10 years ago
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+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5d283663b6d6
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 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.