29276 - text inside <label> tags within a table cell breaks out of table cell

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect, P1)

Description

[Jacob Kjome]

First, I realize that there is another bug that is very similar to this ( 
http://bugzilla.mozilla.org/show_bug.cgi?id=2051 ), but this is more specific 
and I couldn't get rid of the "Worksforme" status...


When text is inside <label> tags within a table cell, the text breaks out of 
the table cell as if the table cell didn't exist.

I have been and am still seeing this in current builds (most recently in Feb 
24, 2000 build).  Take a look at my examples here:

This one really shows off the fact that the problem is with the <label> tag:
http://206.9.170.23/exp/mozTableTest3.html

(Note: I ripped out the check boxes that the label id points to, but that is 
what the <label> tags were used for.  They can be seen in the examples below)


These examples show a simulation closer to what we actually use on a production 
site:
http://206.9.170.23/exp/mozTableTest.html
http://206.9.170.23/exp/mozTableTest2.html

The last two examples were done to replicate the table structure in this site:
http://www.agweb.com

If there is a current survey question available there, you will see this 
behavior.

Please fix this.  It has been around for a while, I just didn't realize it was 
because of <label> tags before.  I finally narrowed it down. I tried a few 
other combinations of HTML elements surrounding text in table cells and didn't 
see this behavior in those unless the <label> tag was also there.  I didn't do 
an exhaustive check, so other people might want to try other elements to try to 
reproduce this behavior.

Jake

Comment 1

[karnaze (gone)]

Rod, this is either a label or block problem.

Comment 2

[Sean Richardson]

Confirmed with the 2000-03-02-08-M15 nightly binary on WinNT 4.0.
On the bright side, the <LABEL>s work ;-> The bug 2051 problem
does seem fixed.

Comment 3

[rods (gone)]

I had buster take a look with me and we found that the mComputedWidth gets set 
to UNCONSTRAINED somewhere in core layout befoer it gets to the LabelFrame.

Here is a stack dump, note that the TableCell is passing in the correct 
mComputedWidth and by the time it gets to the Label it is reset to 
unconstrained. Buster said send this to you and if it wasn't yours to send it to 
him.

nsLabelFrame::Reflow(nsLabelFrame * const 0x01189580, nsIPresContext * 
0x02517030, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 1213312) line 570
nsLineLayout::ReflowFrame(nsIFrame * 0x01189580, nsIFrame * * 0x00128f18, 
unsigned int & 1213312, nsHTMLReflowMetrics * 0x00000000, int & 0) line 992
nsBlockFrame::ReflowInlineFrame(nsBlockReflowState & {...}, nsLineLayout & 
{...}, nsLineBox * 0x011896c8, nsIFrame * 0x01189580, unsigned char * 
0x00128334) line 4011 + 32 bytes
nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState & {...}, nsLineLayout & 
{...}, nsLineBox * 0x011896c8, int * 0x00128bf4, unsigned char * 0x00128a50, int 
0) line 3895 + 28 bytes
nsBlockFrame::DoReflowInlineFramesAuto(nsBlockReflowState & {...}, nsLineBox * 
0x011896c8, int * 0x00128bf4, unsigned char * 0x00128a50, int 0) line 3836 + 38 
bytes
nsBlockFrame::ReflowInlineFrames(nsBlockReflowState & {...}, nsLineBox * 
0x011896c8, int * 0x00128bf4, int 0) line 3781 + 28 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x011896c8, int 
* 0x00128bf4, int 0) line 2953 + 25 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2658 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x01189538, nsIPresContext * 
0x02517030, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 1577 + 15 bytes
nsBlockReflowContext::ReflowBlock(nsIFrame * 0x01189538, const nsRect & {x=0 y=0 
width=1026 height=1073741824}, int 1, int 0, int 1, nsMargin & {top=0 right=0 
bottom=0 left=0}, unsigned int & 0) line 449 + 45 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineBox * 
0x01189754, int * 0x00129560) line 3538 + 59 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x01189754, int 
* 0x00129560, int 0) line 2851 + 23 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2658 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x0118947c, nsIPresContext * 
0x02517030, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 1577 + 15 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x0118947c, nsIPresContext * 
0x02517030, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 
15, int 15, unsigned int 0, unsigned int & 0) line 646 + 31 bytes
nsTableCellFrame::Reflow(nsTableCellFrame * const 0x01189420, nsIPresContext * 
0x02517030, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 694

Comment 4

[rods (gone)]

Attachment: much more reduced case

Comment 5

[troy]

Yes, the computed width is set to NS_UNCONSTRAINEDSIZE, but that's because the 
label frame is an inline non-replaced element (it's not saying it's a replaced 
element anyway)

The CSS 'width' property doesn't apply to inline non-replaced elements (see 
the comment in nsHTMLReflowState.h for data member mComputedWidth) and so the 
computed width is set to NS_UNCONSTRAINEDSIZE to signify that the frame can 
choose any width up to the available width. 

Notice that the available width is something small and reasonable like 1025 
twips

I don't know how labels are intended to work, but text frames (which are also 
inline non-replaced elements) use the available width (not the computed width) 
when reflowing and you should do the same

Now if that's not how labels should work and they're more like a replaced 
element instead, then we'll need to make sure they are marked as replaced 
elements

Comment 6

[Jacob Kjome]

I'm looking at it from a basic web developer perspective and not a programmer 
of a browser, but it seems to me that, no matter what, any element within a 
table cell should, by default, stay within that table cell's borders.  In the 
case where you want to override that behavior with some sort of css or 
javascript (DHTML), then fine, but I can't think of any case where it would be 
beneficial to, by default, break out of the table cell.  That seems completely 
non-intuitive.

In the case of <label>, all it is supposed to do is be a descriptor element 
marking a specific set of text to be related to a form element (usually a 
checkbox or radio button). I can't think of any reason why <label> should tell 
the browser how to display the text within itself.

Am I wrong here?

Jake

Comment 7

[rods (gone)]

inline element must not do anything with mComputedWidth/height

Comment 8

[rods (gone)]

setting availabe size for kid reflow to reflow state available size instead of 
mComputedSize - fixed

Comment 9

[Stephen Walker]

marking verified using 2000051220 build

Comment 10

[Jacob Kjome]

Ok, big problem.

There seems to be a regression here.  Take a look at the testcase for this bug.
 I have not seen this happening until recently.

Please don't let this go to RTM with this regression!!!

Jake

Comment 11

[rods (gone)]

adding buster

Comment 12

[rods (gone)]

Attachment: a little more redcued (no floaters)

Comment 13

[buster]

simple fix: we were doing math on NS_UNCONSTRAINEDSIZE, which is illegal.
NS_UNCONSTRAINEDSIZE is a reserved value that means "infinity" to layout
objects.  This led us to over-eagerly optimize away a required reflow.

patch to be attached in a moment.

nominating for rtm.  without this patch, <LABEL> inside of a <TABLE> will never
work correctly if the content of the <LABEL> is intrinsically wider than the
table cell it is contained within. extremely low risk.  all that is happening is
<LABEL> is getting an addition request to reflow itself, no new code paths, no
new flow-of-control, no changes to allocation patterns, etc.

Comment 14

[buster]

Attachment: tiny test case

Comment 15

[buster]

Attachment: proposed fix

Comment 16

[buster]

rod: please review

Comment 17

[rods (gone)]

Look like exactly the thing to do. r=rods

Comment 18

[karnaze (gone)]

Adding rtm+

Comment 19

[Jason Eager]

Adding regression keyword.

Comment 20

[Phil Peterson]

PDT marking [rtm-] because label problems don't have a very broad effect. Good
fix for the trunk.

Comment 21

[Jacob Kjome]

Does "Trunk" mean it won't make it to RTM????

How can that be.  This was fixed long ago and seems to be a very basic part of 
supporting standards.  How can this not go to RTM?  Honestly, if this browser 
isn't near perfect, developers are going to reject this outright.  Delay the 
thing if you have to, but don't leave out the basics just to meet a deadline.

Jake

Comment 22

[buster]

PDT gave this a -, so Future.
If someone wanted to land the patch on the trunk before I get to it, that would
be ok with me.

Comment 23

[ekrock's old account (dead)]

Clearing [rtm-], clearing Future, and restoring [rtm+] to trigger a
reconsideration here by PDT. Refusing to accept this low-risk, reviewed patch is
the wrong decision for the following reasons:

1) preventing use within any web content by anyone of feature of the standard:
Failing to accept this patch will render LABEL within table cells unusable on
the web for as long as Netscape 6 has significant market share. Web designers
will not be able to include LABELs within their table cells (even for the
purpose of viewing and use on other browsers or other non-visual user agents
such as speech synthesizers) so long as Netscape 6 RTM is causing that LABEL
text to overwrite other cells, rendering the table illegible to able-bodied
users. This is a classic example of a bug in one browser rendering a part of a
specification unusable to everyone on the Internet for years to come.

2) disabled accessibility: The LABEL element has a variety of intended uses, one
of which is making web forms, tables, etc. accessible to the visually impaired.
Note the HTML 4.01 specification:

http://www.w3.org/TR/html4/interact/forms.html#h-17.9.1
"Labels may be rendered by user agents in a number of ways (e.g., visually, read
by speech synthesizers, etc.)"

Failing to fix this bug would impair the ability of web content developers to
create accessible content on the Internet. TABLEs and FORMs are key obstacles to
content accessibility on the Internet, and LABEL helps solve this problem.
(Contrary to comments earlier in this report, be aware that FORMs are commonly
placed within TABLEs on the Internet in order to achieve desired form control
layout. We have examples of this on DevEdge Online. Therefore, it's by no means
unusual to have FORMs within a TABLE, and such forms need LABELs to be accessible.)

3) low-risk: Relevant engineer, first-line engineer, super-reviewer, and brendan
all have reviewed patch and agree that it is inherently low-risk and unlikely to
trigger regressions.

4) legal liability under the ADA: Under the Americans with Disabilities Act,
software companies are required to make reasonable efforts to make their
products accessible to the disabled. By failing to accept this patch, not only
would we be failing to make content accessible within our own product, but
Netscape 6 would be preventing web content developers from leveraging the
efforts of other browser developers to make their products accessible, and from
making their own web content accessible. Companies that fail to make reasonable
efforts are liable to suit under the ADA. AOL is already the subject of one such
lawsuit; we do not want to create additional legal exposure. Note that both
engineers and product managers are subject to subpoena of testimony in such
legal proceedings, and we have agreement of the engineers working on the bug,
the first-line manager, the super-reviewer, brendan (who brought this bug to my
attention) as well as the relevant PM (me) that it is reasonable to accept this
patch at this point in the development process because it is important and low
risk. Marking P1 because of the Legal liability aspect.

Strongly recommend accepting the patch. PDT: If you are considering minusing
this bug again, please call me and we'll schedule time for me and the concerned
engineers to come together and make the case directly in person to PDT. Thanks!

Comment 24

[Michael La Guardia]

Marking rtm++

Comment 25

[buster]

fix checked into branch

Comment 26

[Jacob Kjome]

I don't see this working on todays build 2000100904 M18 Win32.  It was checked
in this morning, shouldn't we being seeing this working?  What does "branch"
refer to?

Jake

Comment 27

[Brendan Eich [:brendan]]

The patch should go into the Mozilla trunk.

/be

Comment 28

[buster]

fix now checked into trunk as well.

Comment 29

[Christine Hoffman]

Verified fixed on Win, Mac and Linux with 10_11 branch build. Added vtrunk 
keyword.

Comment 30

[Stephen Walker]

marking verified using the 2000101704 trunk build on win2k & removing the 
vtrunk keyword.