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

VERIFIED FIXED in mozilla0.9

Status

()

P3
normal
VERIFIED FIXED
19 years ago
18 years ago

People

(Reporter: ian, Assigned: attinasi)

Tracking

({css1, css3})

Trunk
mozilla0.9
css1, css3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: (py8ieh:testcase needs updating on hixie.ch), URL)

Attachments

(5 attachments)

(Reporter)

Description

19 years ago
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?
I'm surprised this doesn't work.  I thought it was tested in the boxacidtest...
Should buster get this bug?

Comment 3

19 years ago
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.
Assignee: pierre → buster
The spec says the numbers are in the right place, but perhaps that's a bit
silly.

Comment 5

19 years ago
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.
Assignee: buster → attinasi
When fixing this, make sure you never change a display of none to block.  See
section 9.7 of CSS2.
(Assignee)

Comment 7

19 years ago
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?
Status: NEW → ASSIGNED
Target Milestone: --- → M16
(Assignee)

Comment 8

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

Comment 10

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

Comment 12

19 years ago
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?
(Assignee)

Comment 13

19 years ago
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.
Keywords: nsbeta2
Whiteboard: fix in hand

Comment 14

19 years ago
[nsbeta2+] [6/1]  Will take fix by 6/1
Whiteboard: fix in hand → [nsbeta2+] [6/1] fix in hand
(Assignee)

Comment 15

19 years ago
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?
(Reporter)

Comment 16

19 years ago
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.
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.
(Reporter)

Comment 18

19 years ago
You'd better post to www-style then...
Whiteboard: [nsbeta2+] [6/1] fix in hand → [nsbeta2+] [6/1] fix in hand (May need clarification from WG)
(Assignee)

Comment 19

19 years ago
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?
(Assignee)

Comment 20

19 years ago
Created attachment 9034 [details] [diff] [review]
Patch for setting display to block for floats and abs. positioned elements: need clarification on tables
(Assignee)

Comment 21

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

Comment 22

19 years ago
Created attachment 9218 [details] [diff] [review]
Patch that does not mess up tables (less strict adherance to CSS spec but won't hork tables)
(Assignee)

Comment 23

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

Comment 24

19 years ago
Created attachment 9433 [details]
testcase showing that tables are still correctly handled
(Assignee)

Comment 25

19 years ago
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'.
Keywords: nsbeta2
OS: Windows 98 → All
Hardware: PC → All
Summary: 'float' should make 'display' take the value 'block' [FLOAT] → Floated elements that are blocks are not having their display set to 'block'
Whiteboard: [nsbeta2+] [6/1] fix in hand (May need clarification from WG) → (Need clarification from WG)
Target Milestone: M16 → Future
(Reporter)

Comment 26

19 years ago
As per meeting with ChrisD yesterday, taking QA.
QA Contact: chrisd → py8ieh=bugzilla
(Reporter)

Comment 27

18 years ago
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.
Keywords: correctness, css3, qawanted
Whiteboard: (Need clarification from WG) → (py8ieh:testcase needs updating)
(Assignee)

Comment 28

18 years ago
Thanks, Ian. Given that information, I believe this bug is closed as fixed,
since the implementation I checked in is as you described it.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Reporter)

Comment 29

18 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 30

18 years ago
Oops - you are right to reopen it, Ian, thanks. I missed the list-item display
when I checked your list. 
Status: REOPENED → ASSIGNED
(Assignee)

Comment 31

18 years ago
Moving to 0.9 as the fix is so easy - will attach soon. Also, I updated the
summary to reflect the remaining problem.
Summary: Floated elements that are blocks are not having their display set to 'block' → Floated elements with display: list-item are having their display set to block when they should not
Target Milestone: Future → mozilla0.9
(Assignee)

Comment 32

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

Comment 33

18 years ago
Created attachment 23989 [details] [diff] [review]
Patch to preserve display:list-item for floated list items
(Assignee)

Comment 34

18 years ago
OK - nsBlockFrame::RenumberListsFor is not handling placeholder frames, so it is
being skipped.
(Assignee)

Comment 35

18 years ago
Created attachment 24003 [details] [diff] [review]
Patch to handle floated list items in nsBlockFrame
(Assignee)

Comment 36

18 years ago
I have attached a patch that makes floated list items renumber properly. Buster
has reviewed it.
(Assignee)

Comment 37

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

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

Comment 39

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

Comment 40

18 years ago
Fix checked in: nsBlockFrame.cpp (3.414) and nsStyleContext.cpp (3.151)
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Reporter)

Comment 41

18 years ago
VERIFIED
Status: RESOLVED → VERIFIED
Keywords: qawanted

Comment 42

18 years ago
hixie: has this regressed?

Comment 43

18 years ago
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...
waterson:  Read the comments starting from 2000-09-18.
(Reporter)

Comment 45

18 years ago
waterson: No, it's fine. Testcase updated (on bath.ac.uk).
Whiteboard: (py8ieh:testcase needs updating) → (py8ieh:testcase needs updating on hixie.ch)
You need to log in before you can comment on or make changes to this bug.