20163 - 'text-decoration' should not apply to text-less inlines (underline) [INLINE]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Pete Closs]

I've noticed this problem before with NMS 4.7 and hoped it would be fixed for
Mozilla, but it seems not. Basically, if you have a link like so: <a
href="http://www.urlhere.com">
<img src="images.gif" border=0>
</a>

A short line appears next to the image. This is only solved when you put the
above 3 tags on the same line. I'm not sure if the html spec says they should go
on one line, but even if it doesn't putting them on 3 seperate lines should not
produce the page spoiling effect of a little ine next to them. An example of
this problem can be seen at the above adress in the menu on the left

Comment 1

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

Changing component to Parser.

My memory is that newlines should be eaten inside tags, but not spaces (although
I wish spaces were too...).  I'd have to check, though.

Comment 2

[rickg]

Not quite david; newlines are meaningful, because the parsing engine can't know
whether the elements that contain them are being treated stylistically as PRE.
The layout engine needs to resolve this later.

Comment 3

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

I meant only *immediately* inside a tag, so that really the following should be
equivalent:

<pre>text
text</pre>

<pre>
text
text
</pre>

but both give two lines.  (Note no spaces in the first case.)

Comment 4

[Mats Palmgren (inactive)]

I think this is a bug too. HTML 4.0 Specification, Appendix B, says:
"SGML (see [ISO8879], section 7.6.1) specifies that a line break immediately
following a start tag must be ignored, as must a line break immediately before
an end tag. This applies to all HTML elements without exception."

http://www.w3.org/TR/REC-html40/appendix/notes.html#notes-line-breaks

Comment 5

[leger]

did you'all want to reopen this?

Comment 6

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

OK.  Reopening for reconsideration in light of the quote from the spec above.

Comment 7

[leger]

Clearing WON'T FIX resolution due to reopen.

Comment 8

[Hixie (not reading bugmail)]

According to CSS, 'text-decoration' with no textual content in it should not
have any effect anyway. Section 16.3.1 of CSS2 says:

# If the element has no content or no text content (e.g., the IMG
# element in HTML), user agents must ignore ['text-decoration'].

So in the case of:

   <a>
          <img ...>     <!-- lots of spaces -->
   </a>

...there should be absolutely no underlining.

Comment 9

[rickg]

I agree with Ian. Reassinging to pierre.

Comment 10

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

I'm not sure I like Ian's interpretation of the spec.  I'm not sure what the
sentences "If it is specified for (or affects) an inline-level element, it
affects all boxes generated by the element. If the element has no content or no
text content (e.g., the IMG element in HTML), user agents must ignore this
property." mean.  Are you sure they're not saying that it only applies to
non-replaced inline elements or replaced inline elements that are text.

There's still a parser bug, in any case.

Comment 11

[Hixie (not reading bugmail)]

David: I like my interpretation, because it is somewhat backwards compatible.
If it were not for that part of the spec, then *the image itself* ought to be
underlined, since its inline parent block has 'text-decoration: underline'.

See http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/underline-img.html
for a test case, and see bug 1777 for more discussion of this.

If, on the other hand, you want this to be about the issue of newlines around
tags, then it is a duplicate of bug 2750.

Comment 12

[Hixie (not reading bugmail)]

David: I've thought more about the spec quote you give:
# If it is specified for (or affects) an inline-level element, it affects all
# boxes generated by the element. If the element has no content or no text
# content (e.g., the IMG element in HTML), user agents must ignore this
# property.
...and I am now even more convinced than ever that this means that textless
inlines are not underlined at all, and those with text underline all their
children, even those without text. I don't see any other self-consistent way
of interpreting the section, and this interpretation makes a lot of sense, is
largely backwards compatible, *and* looks nice. See bug 1777. (It has a
reference screenshot which looks nicer than legacy rendering...)

Pierre: Bug 1777 covers the rendering side of this, and is assigned to Kipp.
That fix should, AFAICT, simply be to apply the *-lining to the element which
has 'text-decoration' set, by drawing a line at a fixed offset from the
baseline (e.g. -0.01em for underline, +0.5em for strike-out, and +1.0em for
overline), and not doing anything special individually for the children.

However, that will not fix the problem of text-less inlines, so I am going to
leave this bug open for you to change the CSS side of things. In particular,
the text-decoration property should not apply to elements that do not have
any textual content. (I assume this includes taking into consideration the
content that :before and :after might give it.)

Some relevant tests:
   http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/underline-img.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/underline.html

Comment 13

[Hixie (not reading bugmail)]

Migrating from {css1} to css1 keyword. The {css1}, {css2}, {css3} and {css-moz}
radars should now be considered deprecated in favour of keywords.
I am *really* sorry about the spam...

Comment 14

[Hixie (not reading bugmail)]

Here is a clearer test case for this exact bug:
 http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/underline-img-2.html

Since the URL given did not appear to show the problem in question, I am 
replacing it with this test case's URL.

chrisd: I am very familiar with this bug, so if you want to defer QA contact
to me, feel free...

Comment 15

[Christine Hoffman]

Ian - I put you on as QA contact and cc'ed me. Thanks

Comment 16

[Pierre Saslawsky]

Pushing again: M18.

Comment 17

[Pierre Saslawsky]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another known 
resource will be working on this bug, or if it blocks your work in some way -- 
please attach your concern to the bug for reconsideration. 

Comment 18

[ekrock's old account (dead)]

Marking relnote. Note that a workaround is to put all the tags on the same line 
in the HTML file.

Comment 19

[Karl Ove Hufthammer]

| This bug has been marked "future" because
| the original netscape engineer working
| on this is over-burdened. If you feel this is an error, [...]

I feel this is an error ...

'text-decoration' is a part of CSS 1, and I think it's important for Mozilla to support this properly.

Comment 20

[Hixie (not reading bugmail)]

Pierre, how difficult is this to fix? As far as I can tell, all you have to do
is check to see if the element (frame?) in question has any textual content, and
if not, force the computed value of 'text-decoration' to be 'none'.

I don't think this is that important, it can probably wait till 5.1.

Comment 21

[Pierre Saslawsky]

I marked it "future" because we MUST push things NOW if we want to ship this year 
and this bug is only a minor annoyance.

Comment 22

[timeless]

You're not doing a good job as qa contact, i'm asking for a replacement =p

Comment 23

[Hixie (not reading bugmail)]

timeless: You're welcome to it.

Comment 24

[Pierre Saslawsky]

I don't like having Style bugs assigned to [e-mail]. People who are 
interested in volunteering some time should get in touch with Marc or myself.

Comment 25

[Mats Palmgren (inactive)]

*** Bug 46170 has been marked as a duplicate of this bug. ***

Comment 26

[Henrik Gemal]

Attachment: Test case for showing small line after image

Comment 27

[Hixie (not reading bugmail)]

As per meeting with ChrisD yesterday, taking back QA. Sorry timeless.

Removing from nsbeta3 radar. This is a low priority bug, we have much more
important issues to fix by RTM if we are going to ship this year.

Comment 28

[ekrock's old account (dead)]

Nom. nsbeta1. Correctness of CSS1 standards compliance.

Comment 29

[Christopher Hoess (gone)]

Adding testcase keyword...

Comment 30

[basic]

Attachment: another testcase

Comment 31

[Pierre Saslawsky]

I don't see how we can fix it in Layout in an efficient way; I agree with dbaron 
[1999-11-30 12:28] that newlines should be eaten by the parser; and I disagree 
with hixie [2000-06-13 12:07] that we could force the computed value of 'text-
decoration' to 'none'. 

When running the latest testcase, nsTextFrame::PaintAsciiText() is called to 
paint a single space and to do so, it calls DrawString2() and 
PaintTextDecorations().  The problem is:
1) We can't avoid to paint the underline in PaintTextDecorations() because we 
don't know what the text is - we are just given a length in twips to draw the 
underline.
2) Not calling PaintTextDecorations() would require to:
  - query the style context and make sure we are not in a preformatted block
  - scan the string and make sure it only contains spaces

My opinion is that it would be too costly to do that everytime we paint a string.
What do you think? WontFix? Reassign to Parser? Something else?

Comment 32

[harishd]

This shouldn't be too difficult ( I think :-\ )to fix in the parser. Taking the bug.

Comment 33

[harishd]

Attachment: Testcase [ I don't think the above testcases are correct ]

Comment 34

[harishd]

Attachment: Tescase [ forgot to update img src in my previous test case ]

Comment 35

[harishd]

Attachment: patch v1.0

Comment 36

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

That still doesn't fix our compliance to the section of CSS2 mentioned by Ian
above (1999-12-20 17:05).  And what about mac line endings?  And shouldn't one
of the kLF's be a kCR, or something like that?  And isn't the relevant rule in
the HTML spec symmetric between after start tags and before end tags?  And could
we really do that in quirks mode without breaking things?

Comment 37

[basic]

[e-mail] : could you explain what is wrong with the first 2 testcases?

Comment 38

[harishd]

dbaron: The patch is only to strip off CR/LF ( though I'm not sure if CR-LF's,
in this case, should be compressed or not ) immediately following a start tag.
Stripping 'em off before the end tag could be tricky. Anyway, I have to refer to
the spec..

Comment 39

[Christopher Hoess (gone)]

The parser bits of this bug are covered by bug 2750. Whether or not we can get
away with this in quirks mode is another story...and, as noted, there's still
the issue of the CSS2 spec (this will just conceal the problem with that).

Comment 40

[Hixie (not reading bugmail)]

Harish: your patch doesn't fix this bug. What it achieves would be better done
by fixing bug 2790. This bug is about not underlining elements that have no
text, not fixing some edge cases involving newlines. Please see comment 8 and
the test in comment 14.

This should probably be assigned to hyatt like bug 1777.

Comment 41

[Hixie (not reading bugmail)]

*** Bug 89224 has been marked as a duplicate of this bug. ***

Comment 42

[Esben Mose Hansen]

Modifying the patch of 1777 to take this into account should amount to checking
whether any found text-frame descendents actually contain any whitespace, as
defined by nsTextFrame::isEmpty(). I'll do this later today. I'll atttach the
patch to bug 1777.

Comment 43

[Esben Mose Hansen]

Duh! s/any whitespace/only whitespace/ Sorry about that, and about the spam...

Comment 44

[Rainer Bielefeld]

Please have a look on bug 126259, too!

Comment 45

[Boris Zbarsky [:bzbarsky]]

Bug 1777 fixed this for standards mode.  It's still broken in quirks mode,
though... is that by-design-ish?

Comment 46

[Hixie (not reading bugmail)]

Indeed:
   http://www.damowmow.com/playground/bugs/tests/underline-img.html

For quirks mode I say we leave it as is unless someone finds a reason to change
it. Marking FIXED by the patch in bug 1777.

Comment 47

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/39c623b0496c
[fenix] possibly fix if statement in sync-strings (https://github.com/mozilla-mobile/fenix/pull/20163)