32200 - Floated elements with display: list-item are having their display set to block when they should not

Status: VERIFIED FIXED

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

Description

[Hixie (not reading bugmail)]

When 'float' does not have the value "none", the element should use the value
'block' for the 'display' property, regardless of what it actually is.

Test case -- see this page:
   http://www.bath.ac.uk/%7Epy8ieh/m/float-list-item.html

You can see that the floated element, which has "display:list-item", is not
being treated as "display:block" as it has a number associated with it (0).

David: At some point someone was arguing that what I say above is actually
incorrect (or silly or something). Do you recall if the WG made a decision
one way or the other?

Comment 1

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

I'm surprised this doesn't work.  I thought it was tested in the boxacidtest...

Comment 2

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

Should buster get this bug?

Comment 3

[Pierre Saslawsky]

Ian:
- What you say is not silly: a 'float' should be rendered as 'display:block' 
unless it has 'display:none'.
- In response to the question raised in the testcase: I think that the numbering 
should be done like in MacIE5 where numbers 3 and 4 appear at the right of the 
'float', not at the left as Mozilla does, because there are other ways to get to 
the same display as Mozilla while there aren't other ways to display like MacIE5.

Reassigned to buster as it is a layout problem. I think that we should first fix 
the problem with 'display' and if we have time, the problem with the location of 
the list item numbers.

Comment 4

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

The spec says the numbers are in the right place, but perhaps that's a bit
silly.

Comment 5

[buster]

Marc and I talked about this, and we think it should be handled during style 
resolution, not by special case hackery in the layout system.  Re-assigning to 
Marc.

Comment 6

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

When fixing this, make sure you never change a display of none to block.  See
section 9.7 of CSS2.

Comment 7

[Marc Attinasi]

We can change the display property when float is set, not too difficult. We have 
to make sure to ignore the setting of display when float is already set too (DOM 
give access to the properties I think) and also make sure setting float and 
display is handled correctly. Also, I wonder if we are going to cause any 
problems because we are losing the original display value...

Marking M16 because this is css1 - should this be pushed on nsbeta2 radar as 
well, being that we claim css1 compliance?

Comment 8

[Marc Attinasi]

I wonder if I should take care of the related case where 'position' is set to 
'absolute' or 'fixed': we should then set 'display' to 'block' and 'float' to 
none... I think it is equally important (also from CSS2 9.7).

Comment 9

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

I suspect we already handle that one correctly (maybe in frame construction),
although I'd have to check.  (Could that be a better place to do this?)

Comment 10

[Marc Attinasi]

I think it is best to have the style context values be consistent with the 
specification. I wouldn't want a bunch of frame classes to have to know about 
these rules and implement special-casing. Also, we need to make sure that DOM 
access leaves the style context's consistent, so where better than at the style 
context itself? (that said, the StyleContext encapsulation is broken by 
GetMutableStyleData so the consistentcy can be comprimised...)

Even if there is code that handles the case of absolutely positioned elements, I 
would like the style context data to be correct. Again, we need to make sure it 
stays consistent.

Comment 11

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

Is the style context maintaining cascaded values or computed values?  I thought
it was the former.  I think the constraint in 9.7 is really a constraint on
computed values, not cascaded values.

Comment 12

[Marc Attinasi]

I'm not sure exactly about the terms 'computed' and 'cascaded' represent here, 
but the StyleContext has the 'inherited' value. That is, if a property is 
specified as 'inherit' for an element, that element's style context will have 
the value copied (inherited) from the parent, not the value 'inherit'. Thus, the 
style context has the correct values for determination of display, position, and 
floats (although it does not have the *computed* values for things like 
width:30% since it cannot know the actual width).

The constraint in 9.7 has to be on the inherited (computed?) value, and for 
properties like display, position and float we have that in the style context.

Interestingly, the note at the end of 9.7 says that CSS2 does NOT specify the 
layout behavior of property changes made by scripts (DOM) - I guess they thought 
that woudl be too difficult. I wonder if CSS3 will specifiy that behavior?

Comment 13

[Marc Attinasi]

Nominating as nsbeta2 because this is a css1 problem, and we are suppossed to be 
css1 compliant. Also, I have a fix for this already.

Comment 14

[Peter Trudelle]

[nsbeta2+] [6/1]  Will take fix by 6/1

Comment 15

[Marc Attinasi]

I was just talking to karnaze about this change and I am confused. The spec 
indicates that we need to make the diplay 'block' if float is set and display is 
not 'none'. This means a floated table will get display-type 'block' and will no 
longer be a table. Is this really what we want, or do we want to make the 
display a block-type (i.e. make sure display is a value that represents a block 
but not necessarily 'block')? I'm guessing that we don't really want a floated 
table to lose it's table-ness, and that what the spec is really indicating is 
that we need to make sure that any floated elements that are displayed become 
blocks *of some kind*. Ian, David, what id your take on this interpretation?

Comment 16

[Hixie (not reading bugmail)]

My take is that we do indeed want a floated table to become a block, and in most
cases that will make no difference to the layout because a table pseudo element
should be implied by the table layout rules.

Similar things happen when absolutely positioning a table cell -- it gets 
removed from the flow and so other cells move around. I believe there is a bug
open on that issue already.

Comment 17

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

I think the only case where there would be a difference caused by changing
'table' to 'block' is when the element had an explicit width.  Because of this
case, I think the spec should be changed.

Comment 18

[Hixie (not reading bugmail)]

You'd better post to www-style then...

Comment 19

[Marc Attinasi]

David is absolutely correct: when the floated table has a width and we change it 
to 'block' the cells are not laying out correctly - presumably the 
generated-table does not get the width from the 'block'...

Also, when we change the table's display to 'block' that causes an anonymous 
table frame to be created, so we end up with an additional frame. I'd like to 
avoid this if there is no good reason for it. Does anyone know the _intention_ 
of the specification?

Comment 20

[Marc Attinasi]

Attachment: Patch for setting display to block for floats and abs. positioned elements: need clarification on tables

Comment 21

[Marc Attinasi]

I attached a patch that changes the display to 'block' as specified in the CSS2 
spec. The patch may need further handling of tables (or possibly other 
block-types), as noted in the comments.

Comment 22

[Marc Attinasi]

Attachment: Patch that does not mess up tables (less strict adherance to CSS spec but won't hork tables)

Comment 23

[Marc Attinasi]

I have attached a patch that makes non-block-type display values map to block. 
This is not the letter of the CSS spec, but it avoids problems with floated 
tables and possibly floated inline-tables.

Comment 24

[Marc Attinasi]

Attachment: testcase showing that tables are still correctly handled

Comment 25

[Marc Attinasi]

A fix was checked in (nsCSSStyleRule.cpp).

I took a loose interpretation of the spec here: we ensure that the element being 
floated has a display type that is *a* block, not necessarily 'block'. This is 
not to the letter of the spec but keeps tables from getting hosed when they have 
a width specified (probably other block elements too).

Updating the summary to reflect the remaining issue: Floated elements that are 
blocks are not having their display set to 'block' - need WG clarification still 
on this. It may be closed if the CSS spec. is revised in light of the 
potential problems associated with changing a block's display to 'block'.

Comment 26

[Hixie (not reading bugmail)]

As per meeting with ChrisD yesterday, taking QA.

Comment 27

[Hixie (not reading bugmail)]

Clarification was given at the CSS WG F2F.

It was decided that the 'display' property should be set to a block level 
equivalent, so that list-items should retain their bullets, tables should remain
tabular, and so on and so forth. 

Basically:
   block        -> block
   inline       -> block
   list-item    -> list-item
   table        -> table
   inline-table -> table
   table-*      -> table-*

CSS3 will explain this in more depth.

Comment 28

[Marc Attinasi]

Thanks, Ian. Given that information, I believe this bug is closed as fixed,
since the implementation I checked in is as you described it.

Comment 29

[Hixie (not reading bugmail)]

One side effect of the definition agreed at the WG F2F is that floated list
items should retain their bullet/number and affect the numbering of subsequent
list items. If you look at 
   http://www.bath.ac.uk/%7Epy8ieh/m/float-list-item.html
...you will see that we are not doing that (the "fourth item" should have the
number "5" -- I need to update the test case).

REOPENING. Sorry! Future is fine, BTW, given that the CSS WG only recently
agreed to the clarification, and that we are this close to shipping.

Comment 30

[Marc Attinasi]

Oops - you are right to reopen it, Ian, thanks. I missed the list-item display
when I checked your list. 

Comment 31

[Marc Attinasi]

Moving to 0.9 as the fix is so easy - will attach soon. Also, I updated the
summary to reflect the remaining problem.

Comment 32

[Marc Attinasi]

Lovely - when I made the change to keep floated list-item's as display:list-item
the results are strange: the floated item gets the number '0' and the rest
remain 1-4. There is more to this bug than I thought, so I'll attach the patch
to keep the list-item display type from going to block and look into the
numbering problem.

Comment 33

[Marc Attinasi]

Attachment: Patch to preserve display:list-item for floated list items

Comment 34

[Marc Attinasi]

OK - nsBlockFrame::RenumberListsFor is not handling placeholder frames, so it is
being skipped.

Comment 35

[Marc Attinasi]

Attachment: Patch to handle floated list items in nsBlockFrame

Comment 36

[Marc Attinasi]

I have attached a patch that makes floated list items renumber properly. Buster
has reviewed it.

Comment 37

[Marc Attinasi]

The patch doesn't show it, but I added

NS_ASSERTION(aPresContext && aTheKid && aOrdinal, "null params are immoral!");

to the top of nsBlockFrame::RenumberListsFor, since all of these args are
dereferenced without a check...

Comment 38

[Chris Waterson]

Why did you change `aKid' to `aTheKid'? This change is unnecessary and sort of 
weird, give the variable naming conventions in common use. Could you please 
leave as is? Fix that, and sr=waterson

Comment 39

[Marc Attinasi]

> Why did you change `aKid' to `aTheKid'?
I changed it so that I would not mistakenly use 'aKid' instead of 'kid'. I'll be
happy to change it back - no problem at all. What I really don't want to do is
assign the placeholder's OutOfFlow to the argument aKid - that style bothers me.
Thanks, Chris.

Comment 40

[Marc Attinasi]

Fix checked in: nsBlockFrame.cpp (3.414) and nsStyleContext.cpp (3.151)

Comment 41

[Hixie (not reading bugmail)]

VERIFIED

Comment 42

[Chris Waterson]

hixie: has this regressed?

Comment 43

[Chris Waterson]

Specifically, the float is being numbered. (I noticed this when doing
archaeology for bug 42138). Here is what I see:

  1. first item
    +------------+
  2.|float     3.|second item
    +------------+
               4. third item

  5. fourth item

From the verbiage of the test case, I expected that ``float'' would not be
numbered...

Comment 44

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

waterson:  Read the comments starting from 2000-09-18.

Comment 45

[Hixie (not reading bugmail)]

waterson: No, it's fine. Testcase updated (on bath.ac.uk).