21415 - [INLINE] Empty inlines not taking space if alone on a line {ll}

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[sujay]

using 12/10 build of apprunner (1999121008)

1) launch apprunner
2) launch editor
3) immediately insert an anchor

where is the anchor? you don't see it.

4) start typing some text

now you see the anchor

all platforms.

Comment 1

[Charles Manske]

Interesting - its a layout problem. If you type and then delete the text, the
Named Anchor image disappears! Maybe generated content (this image) doesn't
get displayed if it's the only content in the document?

Comment 2

[Charles Manske]

We are using generated content to show an icon for a named anchor, which is not
displayed in the browser, but we need to show something in Composer.
Here is the CSS:
a[name] {
  /* TEMPORARY TO COMPENSATE FOR BUG */
  padding-left: 16px;
  background: url(chrome://editor/skin/images/anchor-in-doc.gif) no-repeat;
  background-position: center center;
}

As described above, if a named anchor is the only content in a document,
the icon doesn't display. I'll add attachments to demonstrate this in viewer:
SoloAnchor.html has just the anchor and no text. In viewer, the page appears
blank.
Anchor.html has the same anchor plus some text and now the named anchor image
shows up in viewer.

Comment 3

[Charles Manske]

Attachment: Test of named anchor as only content in document

Comment 4

[Charles Manske]

Attachment: Named Anchor plus some text

Comment 5

[Charles Manske]

Adding myself to CC.

Comment 6

[troy]

Sounds like a block issue

Comment 7

[buster]

mass moving all Kipp's pre-beta bugs to M15.  Nisheeth and I will
prioritize these and selectively move high-priority bugs into M13 and M14.

Comment 8

[Charles Manske]

I'm asking that this bug receive higher "dogfood" priorty because it messes up
my ability to continue table editing development, i.e., it's "developer
dogfood".
Table editing is an important feature and with the current state, I can't
see if my code is working because the table doesn't redisplay correctly after
inserting or deleting rows and columns when a table has rowspan and/or colspan
> 1.

Comment 9

[Charles Manske]

Attachment: Sample table with some cells having rowspan and colspan > 1

Comment 10

[Charles Manske]

Using the 12/21/99 attachment, here is an example of bad table layout as a
result of not responding the the changed rowspan:
1. Edit the 12/21 sample file
2. Place caret in cell containing "7"
3. Execute menu command Table | Insert | Row Below
See the bad table layout!
Execute Debug | Test Table Layout to show debug information: notice that the
cell with the "6" did have its rowspan changed to 4, as it should.
Note also that you can click in various cells in the table and type or resize
the window, but none of these actions cause the table to redisplay correctly.
If you us File | Save As to save the file locally, then reload that file,
the table will display correctly.

Comment 11

[leger]

Putting on PDT- radar.  We do not think table editing is needed for dogfood.
Putting on [BETA] radar.

Comment 12

[rubydoo123]

removing dogfood markings, adding feature marking and assigning to buster for
m13

Comment 13

[Charles Manske]

Please ignore comments starting with my rant on 1999-12-21 13:02. Comments
starting there were intended for bug 22246. This bug has nothing to do with
tables. Also ignore the "Sample table..." attachment.

Comment 14

[buster]

the <A> is generating a frame that has width and height, so it should paint.  It
isn't painting because the linebox containing it has
mData->mCombinedArea==(0,0,0,0) at the time paint is called.  The linebox
combined area is not 0 if there is anything else on the line.

This is a post-beta problem, not critical.

Also, I don't think it's a feature at all, I think it's just a bug.  Something
in the line reflow code isn't accounting for genreated content properly.

Throwing into Kipp's bucket.

Comment 15

[buster]

kipp is very unlikely to fix these, since he's not working ont he project any 
longer.  so I'll take a look.

Comment 16

[buster]

redistributing bugs across future milestones, sorry for the spam

Comment 17

[buster]

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

[Charles Manske]

This is a very rare situation: Usually, you have some content in the page,
so the only time this happens is when inserting a named anchor is the very first
thing you do. Since the user will have to add some content to the page after 
that, the anchor icon will appear.
So we definitely have other more important things to worry about.

Comment 19

[Charles Manske]

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

Comment 20

[Charles Manske]

This problem is now seen with the "smiley" icons used in Messenger Composer,
which is a more important case than just the named anchor icon.
It would be more common for a user to select a smiley as the first thing in a
message, and thus be confused since it doesn't appear until they start typing.
Removing milestone and reassigning to Marc for consideration for fixing sooner
than "future"

Comment 21

[Marc Attinasi]

I'm confused here. The testcase (attachID=3666) is using a background image, not
generated content. Buster dropped some comments about generated content which I
think are obsolete (?) since the problem now is that the background image is not
being shown if the anchor has no text.

From what I see, the behavior is actually 'correct', although I see that it is
not the desired behavior. According to CSS2 (10.2) "The width of a non-replaced
inline element's boxes is that of the rendered content within them (before any
relative offset of children)"

FWIW, IE does just what Mozilla does (i.e. show nothing) and Nav shows nothing
either except if you put a border on it you can see that Nav does assign some
space to the anchor.

Please bring me up to date on the bug here, especially with respect to generated
content - thanks. Maybe we can find a way to force the behavior you want...

Comment 22

[Charles Manske]

Sorry to confuse, I forgot the using background image isn't "generated content".
So yes, the behavior is technically correct, but we would like the
background icon to appear even if document is empty.

Comment 23

[Marc Attinasi]

As far as I can tell, the issue is really that the block that contains the named
anchor has to have something else in it. For example, I made a doc like:

<body>
<div>
a
</div>
<div>
<a name="hello" style="padding: 30px; background:
url(http://www.mozilla.org/editor/images/img-align-left.gif) no-repeat;
background-position:center center"></a>
</div>
</body>

and it still shows no background image, however

<body>
a
<a name="hello" style="padding: 30px; background:
url(http://www.mozilla.org/editor/images/img-align-left.gif) no-repeat;
background-position:center center"></a>
</body>

dose because the containing block (the body in this case) has something else in
it. Like buster mentioned way back in '99, the frame for the <a> has a size, but
the linebox that contains it is collapsing it for some reason. I'll see if I can
figure it out.

Comment 24

[Marc Attinasi]

OK, digging into the line layout code I saw the reason that the inline with no
content is not combined into the line size, and it is pretty explicit:

from nsLineLayout::RelativePositionFrames(PerSpanData* psd,
                                          nsRect& aCombinedArea):
    // Only if the frame has some area do we let it affect the
    // combined area. Otherwise empty frames placed next to a floating
    // element will cause the floaters margin to be relevant, which we
    // don't want to happen.

I'm really afraid that I'll introduce some (more) floater bugs if I make make it
so that an empty inline takes up space so the background image is shown.

As an alternative for the composer, maybe you can put a style rule into your
override stylesheet that makes, for example, anchors have some generated
content, which makes them display the background image. For example, I put this
into the testcase:
 <style>a:after{white-space:pre;content:" ";}</style>
and it works.

Hack, yes, but can you swallow it? If not, then I'll investigate further the
impact of making the empty inline take up space...

(CC'ing Hixie for his floater expertiese and general wisdom)

Comment 25

[Hixie (not reading bugmail)]

Attachment: Simple and self explanatory testcase. Output should be green, not red.

Comment 26

[Hixie (not reading bugmail)]

Empty inlines *should* be taking space (except in quirks mode). This is a bug. 
I'm not sure what the issue regarding floaters is, though. It sounds suspect. 
Why would we not want floater margins to take effect? 

cc'ing dbaron, taking QA and turning this into a CSS bug. ;-)

Comment 27

[Marc Attinasi]

Thanks Ian. Setting milestone to Moz1.0 - Charley, yell loud if you need it 
sooner...

Comment 28

[Charles Manske]

No, we already tried generated content and the problem is we can't select the
icon, show a selected border, and have it return the <a> tag as the element
clicked on.
I'm fuzzy on the details, but I know we did try hard to use generated
content. Mjudge might remember more details if you want.

Comment 29

[Anatoliy Averbukh]

I have the same problem with smileys (Messenger Composer): they are implemented 
the same way as anchor (that is, 2 spans with hided internal span, the internal 
span has text I'd like to hide and external span had to be represented as a 
background image). I need to have that type of "ascii" images because of 
special requirements I have, but the bug is very confusing for end-users, 
because they insert smileys with menu and watch nothing (if there is no text). 
Please fix that as soon as possiible. Also, pseudo elements don't correctly 
work for me too(they show a strange behavior). Thank you. 

Comment 30

[scalkins]

Nominating nsbeta1 as this affects Im in commercial release per Anatoliya
See Bugscape bug http://bugscape.netscape.com/show_bug.cgi?id=3967

Comment 31

[Marc Attinasi]

OK - Mozilla 0.9

Comment 32

[Hixie (not reading bugmail)]

Wow, an obscure inline box model bug is being prioritised! I like! ;-)

Comment 33

[Marc Attinasi]

Not obscure now - the Editor and AIM need it!

Comment 34

[Hixie (not reading bugmail)]

Just one of the unexpected bonuses of using Gecko for UI! ;-)

Comment 35

[Marc Attinasi]

I have a patch for this, but I cannot get to either of Ian's or David's tests 
tonight for some reason, so I have only tested it lightly. The patch makes 
background images appear even if the inline element is empty though, so it fixes 
this bug. I'll be testing further to make sure it doesn't break anything else 
(help appreciated, as always). 

Oh, and to Ian's point earlier (2001-02-22 18:17): I cannot make this fix 
standard-mode only because the composer creates quirk-mode documents! Ack.

Attaching patch...

Comment 36

[Marc Attinasi]

Attachment: Patch to fix the bug (preliminary testing so far)

Comment 37

[Hixie (not reading bugmail)]

Attachment: Test case - behaviour should not be changed by the patch!

Comment 38

[Marc Attinasi]

The new test that Ian attached renders the same before and after the patch. I
think that placeholders are handled specially in the layout (i.e. they are
treated as placeholders and not added into the lines area) - but I am double
checking that now.

Comment 39

[Hixie (not reading bugmail)]

Testcases I can get to: 
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=25982
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=28258
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight1.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight2.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight3.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight4.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight6.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight7.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/emptyinline.html

Comment 40

[Marc Attinasi]

OK, all of the tests listed are the same with and without the patch (except for
the first which needs the patch to pass). (I could not get the 'Ahem' font
however, so I'm not sure if some of those eviltests were valid or not.)

This patch is ready for review, please.

Thanks so much for the tests, Ian.

Comment 41

[Hixie (not reading bugmail)]

The Ahem font is now at:
   http://style.cleverchimp.com/junk/AHEM____.TTF  (Windows)
   http://style.cleverchimp.com/junk/Ahem.ps       (Unix)
   http://style.cleverchimp.com/junk/Ahem.sit      (Mac)

Comment 42

[Chris Waterson]

In kipp's cvs annotation that looks relevant to this line (r3.41), he mentions
that this fixes bug 12910. Unfortunately, that bug looks like something
completely unrelated, so I suspect bug dyslexia when kipp entered the comment.
Worse, it looks like bugzilla has forgotten all of kipp's resolved bugs (grr),
so I can't even track down what he might have thought he fixed.

Anyway, I'm a bit worried that this is going to regress something...

Comment 43

[David Baron :dbaron:]

All of kipp's bugs were transferred to buster.  (There may even be two
[e-mail] accounts, with one of them being what used to be kipp's...)

Comment 44

[Hixie (not reading bugmail)]

Here are the bugs kipp marked fixed on the day that he checked in revision 3.41
of nsLineLayout.cpp:

bug 4803  Text overflows within table, column border elongated  
bug 7352  Image layouts created by piecing together images in tables  
bug 10496 Broken breaking-spaces should not be rendered  
bug 11932 problems with margin calculations on blocks with wider width than p...
bug 12709 right-floating image makes its cell expand  
bug 13553 mozilla0.8 Table doesn't render properly  
bug 14171 ran viewer under purify, got UMR in nsFrameConstructorState::PushFl...

Here are relevant test cases (in no particular order) from these bugs. None of 
them should change behaviour between being with or without the fix to this bug:

   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=1422
   http://www.prometheus-music.com/gecko/ivillage6.html
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=1550
   http://www.fas.harvard.edu/~dbaron/css/test/sec100303
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=992
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=1831
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=1846
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=1892
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=1893
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=1635
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23496

Assuming none of these break, I would be pretty confident that kipp would be
happy to let us check this in. Of course, we could just call him and ask him
for his advice! ;-)

Comment 45

[Marc Attinasi]

Ian, thanks so much for providing these test cases! I ran each of them with and
without the patch and they look the same - i.e. none of them broke. I am happy
about this ;)

However, I am not ruling out the possibility for other cases that may now break.
I feel that the layout code is pretty fragile (wrt floaters especially), and the
comment about impact to floaters is particularly alerting. However, if we do
regress something with regard to floater placement or margins or whatever, I
think we need to address it correctly and not simply make empty inlines take up
no space, in violation of the CSS1 specification. In other words, I'd be willing
to take the heat and fix any regressions we run across. Chris, Ian, whaddayathink?

Comment 46

[Chris Waterson]

Sounds like a plan. [s]r=waterson

Comment 47

[Hixie (not reading bugmail)]

attinasi: hey, if you want to take the heat I'm all for it. ;-)

Comment 48

[Marc Attinasi]

Table regression tests passed fine: only 5 false positives that all look 
identical. 

karnaze said he would r= if the table regression tests passed, so assuming 
r=karnaze

Comment 49

[Marc Attinasi]

Fixed. nsLineLayout.cpp ver. 3.95

Comment 50

[sujay]

verified in 5/9 build.

Comment 51

[Hixie (not reading bugmail)]

Looks good to me too.