Closed Bug 645647 Opened 13 years ago Closed 8 years ago

list-style-position affects li width even if list-style-type is none

Categories

(Core :: Layout: Block and Inline, defect, P2)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: d7, Assigned: MatsPalmgren_bugz)

References

(
URL
)

Details

(Keywords: testcase)

Attachments

(5 files, 2 obsolete files)

Reporter's Testcase
2.41 KB, text/html
Details
reftests
3.78 KB, patch
Details | Diff | Splinter Review
fix
10.32 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
remove unused trailingTextFrame member
3.17 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
add an 'm' prefix to member names
38.70 KB, patch
dholbert
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0

Hello,
It seems this issue has appeared after I upgraded to firefox 4. It was not an issue in firefox 3.6

The list of images displayed in the URL above overflow because the width of li elements is incorrectly worked out by firefox.

The problem is demonstrated by a simple test webpage:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3c.org/TR/1999/REC-html401-19991224/loose.dtd">
<HTML xmlns="http://www.w3.org/1999/xhtml" xmlns:og = 
"http://opengraphprotocol.org/schema/" xmlns:fb = 
"http://www.facebook.com/2008/fbml">
<head></head>
<body>
<div class="prodViewImage"><a href="http://www.audioaffair.co.uk/images/uploads/big/AEGNP4.jpg"><img width="290" border="0" src="Audioengine%20P4%20Speakers%20(Pair)%20at%20Audio%20Affair_files/AEGNP4.jpg" alt="Audioengine P4 Speakers (Pair)" title="Audioengine P4 Speakers (Pair)"> </a>
    <div class="prodImgs" style="visibility: visible; overflow: hidden; position: relative; z-index: 2; left: 0px; width: 319px;">
      <ul style="margin: 0pt; padding: 0pt; position: relative; list-style-type: none; list-style-position: inside; width: 435px; left: 0px;">
        <li style="text-align: center; margin: 0px 2px; float:left;"><a href="http://www.audioaffair.co.uk/images/uploads/AEGNP42.jpg"><img width="75" height="75" border="0" src="Audioengine%20P4%20Speakers%20(Pair)%20at%20Audio%20Affair_files/thumb_AEGNP42.jpg" alt="Click for Larger Image" title="Click for Larger Image"> </a></li>
        <li style="text-align: center; margin: 0px 2px; float:left;"><a href="http://www.audioaffair.co.uk/images/uploads/AEGNP43.jpg"><img width="75" height="75" border="0" src="Audioengine%20P4%20Speakers%20(Pair)%20at%20Audio%20Affair_files/thumb_AEGNP43.jpg" alt="Click for Larger Image" title="Click for Larger Image"> </a></li>
        <li style="text-align: center; margin: 0px 2px; float:left;"><a href="http://www.audioaffair.co.uk/images/uploads/AEGNP44.jpg"><img width="75" height="75" border="0" src="Audioengine%20P4%20Speakers%20(Pair)%20at%20Audio%20Affair_files/thumb_AEGNP44.jpg" alt="Click for Larger Image" title="Click for Larger Image"> </a></li>
        <li style="text-align: center; margin: 0px 2px; float:left;"><a href="http://www.audioaffair.co.uk/images/uploads/AEGNP45.jpg"><img width="75" height="75" border="0" src="Audioengine%20P4%20Speakers%20(Pair)%20at%20Audio%20Affair_files/thumb_AEGNP45.jpg" alt="Click for Larger Image" title="Click for Larger Image"> </a></li>
        <li style="text-align: center; margin: 0px 2px; float:left;"><a href="http://www.audioaffair.co.uk/images/uploads/AEGNP45.jpg"><img width="75" height="75" border="0" src="Audioengine%20P4%20Speakers%20(Pair)%20at%20Audio%20Affair_files/thumb_AEGNP45.jpg" alt="Click for Larger Image" title="Click for Larger Image"> </a></li>
       </ul>
    </div>
</div>
      
</body>
</html>


Reproducible: Always

Steps to Reproduce:
1. Save the above webpage snippet to a file and open in browser.
2. Observe the image list.
3. In the ul element containing the li image list change list-style-position: inside; to list-style-position: outside;
4. Refresh the page and observe the image list.

Do the same in firefox 3.6
Ignore the previous testpage, it appears broken due to local image URLs.

Please use this:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
"http://www.w3c.org/TR/1999/REC-html401-19991224/loose.dtd">
<HTML xmlns="http://www.w3.org/1999/xhtml">
<head></head>
<body>
<div class="prodViewImage"><a href="http://audioaffair.co.uk/images/uploads/big/AEGNP4.jpg" rel="shadowbox[AEGNP4]"> <img width="290" border="0" title="Audioengine P4 Speakers (Pair)" alt="Audioengine P4 Speakers (Pair)" src="http://audioaffair.co.uk/images/uploads/AEGNP4.jpg"> </a>
	
<div class="prodImgs" style="visibility: visible; overflow: hidden; position: relative; left: 0px; width: 333px;">

<ul style="margin: 0pt; padding: 0pt; position: relative; list-style-type: none; list-style-position: inside; width: 455px; left: 0px;">
    <li style="margin: 0px 2px; text-align: center; overflow: hidden; float: left;"><a href="http://audioaffair.co.uk/images/uploads/AEGNP42.jpg" rel="shadowbox[AEGNP4]">
	<img width="75" height="75" border="0" alt="Click for Larger Image" title="Click for Larger Image" src="http://audioaffair.co.uk/images/uploads/thumbs/thumb_AEGNP42.jpg"> </a></li>
	
	<li style="margin: 0px 2px; text-align: center; overflow: hidden; float: left;"><a href="http://audioaffair.co.uk/images/uploads/AEGNP43.jpg" rel="shadowbox[AEGNP4]">
	<img width="75" height="75" border="0" alt="Click for Larger Image" title="Click for Larger Image" src="http://audioaffair.co.uk/images/uploads/thumbs/thumb_AEGNP43.jpg"> </a></li>
	
	<li style="margin: 0px 2px; text-align: center; overflow: hidden; float: left; "><a href="http://audioaffair.co.uk/images/uploads/AEGNP44.jpg" rel="shadowbox[AEGNP4]">
	<img width="75" height="75" border="0" alt="Click for Larger Image" title="Click for Larger Image" src="http://audioaffair.co.uk/images/uploads/thumbs/thumb_AEGNP44.jpg"> </a></li>
	
	<li style="margin: 0px 2px; text-align: center; overflow: hidden; float: left; "><a href="http://audioaffair.co.uk/images/uploads/AEGNP45.jpg" rel="shadowbox[AEGNP4]">
	<img width="75" height="75" border="0" alt="Click for Larger Image" title="Click for Larger Image" src="http://audioaffair.co.uk/images/uploads/thumbs/thumb_AEGNP45.jpg"> </a></li>
	
    <li style="margin: 0px 2px; text-align: center; overflow: hidden; float: left; ">
	<img width="75" height="75" border="0" alt="" title="" src="http://audioaffair.co.uk/images/uploads/thumbs/blank.jpg"></li>
	
</ul>
</div>
</div>

</body>
</html>
Issue confirmed 

Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Layout
Keywords: testcase
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → Trunk
What's happening here is that bullet frames have an 8px end-margin.  When computing the intrinsic width of the bullet, that margin is taken into account even if the list-style-type is none.  On the other hand, line layout will ignore end margins for any zero-width frames.  This seems bogus, since it doesn't ignore start margins...

The upshot is that we allocate the 8px of space when shrink-wrapping but don't move the text 8px to the right.

It would be pretty simple to fix the former, I suppose....
Component: Layout → Layout: Block and Inline
QA Contact: layout → layout.block-and-inline
Note, in particular, that you get the same issues if you have a <span style="margin-right: 100px"></span> in a shrink-wrap block.
Attached patch Fix (obsolete) — — Splinter Review 

        Attachment #522406 -
        Attachment description: dbaron → Fix

        Attachment #522406 -
        Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Priority: -- → P2
Whiteboard: [need review]

    
    

    
  
Comment on attachment 522406 [details] [diff] [review]
Fix

This breaks list-style-image.  Compare to nsBulletFrame::GetDesiredSize.  (Do any of the callers that currently use nsBlockFrame::BulletIsEmpty also really need that test, or are they ok?)

nsBulletFrame probably also needs to override GetUsedPadding, GetUsedBorder, and GetUsedMargin if it's going to ignore its padding, border, and margin.

That said, I don't understand why we create bullet frames at all in this case... though I suppose we have to create the bullet frame for list-style-type:none and a non-none list-style-image.

        Attachment #522406 -
        Flags: review?(dbaron) → review-

    
    

    
  
> This breaks list-style-image. 

Hmm...  Good point.

What do you think of just not using a margin here and instead changing GetDesiredSize to add on the extra space instead?

> Do any of the callers that currently use nsBlockFrame::BulletIsEmpty also
> really need that test, or are they ok?

Not sure.

> That said, I don't understand why we create bullet frames at all

We just always create them for the first-in-flow of a display:list-item.
Whiteboard: [need review]

    
    

    
  
I suppose we could condition that part on list-style-type and list-style-image, but then changes to those would need to reframe....

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix (obsolete)
        — 
          — Splinter Review
      

    


  
Hope you don't mind if I steal this...

The problem is that InlineIntrinsicISizeData::skipWhitespace is set to false
for all bullet frames, which prevents nsTextFrame from trimming leading
whitespace during intrinsic sizing.
http://hg.mozilla.org/mozilla-central/annotate/06678484909c/layout/generic/nsTextFrame.cpp#l7757

The fix is to ignore zero-sized bullet frames with list-style-type:none
(and no image).
Assignee: bzbarsky → mats

        Attachment #522406 -
        Attachment is obsolete: true

        Attachment #8739269 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 8739269 [details] [diff] [review]
fix

I'm going to punt this to David, but.... given comment 10, why aren't we just setting aData->skipWhitespace to true either always or sometimes?

        Attachment #8739269 -
        Flags: review?(bzbarsky) → review?(mats)

    
    

    
  
Comment on attachment 8739269 [details] [diff] [review]
fix

(In reply to Boris Zbarsky [:bz] from comment #12)
> I'm going to punt this to David, but.... given comment 10, why aren't we
> just setting aData->skipWhitespace to true either always or sometimes?

OptionallyBreak() / ForceBreak() does that bit:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=a5d8c5b65333#4295

I don't think we need to change any flags for the ignorable case.

dbaron isn't accepting review requests, so punting to dholbert.

        Attachment #8739269 -
        Flags: review?(mats) → review?(dholbert)

    
    

    
  
Looks like this ends up duplicating a bunch of code between the nsBulletFrame and nsFrame impls here. (The new impls look exactly like the nsFrame impls, except for the initial check, and the unconditional trailingTextFrame=false assignment in nsBulletFrame::AddInlineMinISize.)

Could we avoid this code-duplication, say by spinning out a non-virtual helper-method in nsFrame.cpp, which would be called by the nsFrame and nsBulletFrame impls?

I'm imagining that method would be something like:
 void
 nsFrame::AddInlineMinISize(nscoord aISize,
                            nsIFrame::InlineMinISizeData* aData)
 {
   // Current nsFrame::AddInlineMinISize code all moves to here,
   // except for the isize computation
 }

And then the existing nsFrame::AddInlineMinISize method would become:
 void
 nsFrame::AddInlineMinISize(nsRenderingContext *aRenderingContext,
                            nsIFrame::InlineMinISizeData *aData)
 {
    nscoord isize = nsLayoutUtils::IntrinsicForContainer(...);
    AddInlineMinISize(isize, aData);
 }

...and your new nsBulletFrame impl would be similar to that ^^, except it would include your special case first (checking isize for nonzeroness) and it would also set trailingTextFrame = nullptr up-front unconditionally (somewhat redundantly but that's not a big deal).
Flags: needinfo?(mats)

    
    

    
  
Also: a brief code-comment to explain the special case would be helpful here.

e.g.: "If a bullet has nonzero size and is "ignorable" from its styling, we behave as if it doesn't exist, from a linebreaking/isize-computation perspective. Otherwise, we match the standard nsFrame impl, because bullets don't participate in line breaking."

Might even make sense to refactor your current IsIgnorable function to make it take a nscoord arg (for the computed isize), and then you can place something like this comment alongside that method. And your nsBulletFrame Add...ISize impls would then just have:
   nscoord isize = ...;
   if (!IsIgnorable(this, isize) {
     nsFrame::Add...ISize(isize, ...);
   }
...which would be pretty self-documenting & grokkable.

    
    

    
  
Comment on attachment 8739269 [details] [diff] [review]
fix

So, this is r- for now, but likely r+ with comments 14 & 15 addressed somehow.

        Attachment #8739269 -
        Flags: review?(dholbert) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix
        — 
          — Splinter Review
      

    


  
OK, I refactored the code so we can share a default impl between nsFrame and
nsBulletFrame.  I also changed nsImageFrame::AddInlineMinISize to use
the same code.  Note that I made one change in the latter though:
-    parent->StyleText()->WhiteSpaceCanWrap(parent) &&
is essentially replaced with (in the shared code):
+    !aParentFrame->StyleContext()->ShouldSuppressLineBreak() &&
+    aParentFrame->StyleText()->WhiteSpaceCanWrap(aParentFrame);

I think ShouldSuppressLineBreak() part was an unintentional omission
in nsImageFrame (other than that, this is just a refactoring from
the last version).

        Attachment #8739269 -
        Attachment is obsolete: true
Flags: needinfo?(mats)

        Attachment #8740754 -
        Flags: review?(dholbert)

    
    

    
  


  

        Attachment #8740755 -
        Flags: review?(dholbert)

    
    

    
  


  

        Attachment #8740756 -
        Flags: review?(dholbert)

        Attachment #8740755 -
        Flags: review?(dholbert) → review+

        Attachment #8740756 -
        Flags: review?(dholbert) → review+
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: list-style-position effects li width even if list-style-type is none → list-style-position affects li width even if list-style-type is none

    
    

    
  
Comment on attachment 8740754 [details] [diff] [review]
fix

Review of attachment 8740754 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following addressed (let me know if you can think of a better way to address my main comment here about DefaultAddInlineMinISize/aCanBreak/mayBreak

::: layout/generic/nsBulletFrame.cpp
@@ +701,5 @@
> +{
> +  nscoord isize = nsLayoutUtils::IntrinsicForContainer(aRenderingContext,
> +                    this, nsLayoutUtils::MIN_ISIZE);
> +  if (MOZ_LIKELY(!::IsIgnoreable(this, isize))) {
> +    aData->DefaultAddInlineMinISize(GetParent(), !CanContinueTextRun(), isize);

Per my comment below about the DefaultAddInlineMinISize API, I think this caller may want to change to simply:
  aData->DefaultAddInlineMinISize(this, isize);

(See comment below for more details)

@@ +709,5 @@
> +/* virtual */ void
> +nsBulletFrame::AddInlinePrefISize(nsRenderingContext* aRenderingContext,
> +                                  nsIFrame::InlinePrefISizeData* aData)
> +{
> +  nscoord isize = nsLayoutUtils::IntrinsicForContainer(aRenderingContext, 

Fix EOL whitespace here.

::: layout/generic/nsFrame.cpp
@@ +4261,5 @@
> +                           nsIFrame::InlineMinISizeData* aData)
> +{
> +  nscoord isize = nsLayoutUtils::IntrinsicForContainer(aRenderingContext,
> +                    this, nsLayoutUtils::MIN_ISIZE);
> +  aData->DefaultAddInlineMinISize(GetParent(), !CanContinueTextRun(), isize);

Per my comment below about the DefaultAddInlineMinISize API, I think this caller may want to change to simply:
  aData->DefaultAddInlineMinISize(this, isize);

(See comment below for more details)

@@ +4268,5 @@
>  /* virtual */ void
> +nsFrame::AddInlinePrefISize(nsRenderingContext* aRenderingContext,
> +                            nsIFrame::InlinePrefISizeData* aData)
> +{
> +  nscoord isize = nsLayoutUtils::IntrinsicForContainer(aRenderingContext, 

Fix EOL whitespace here.

@@ +4280,5 @@
> +                                                       nscoord   aISize)
> +{
> +  MOZ_ASSERT(aParentFrame, "Must have a parent if we get here!");
> +  const bool mayBreak = aCanBreak &&
> +    !aParentFrame->StyleContext()->ShouldSuppressLineBreak() &&

Hmm, two concerns here:
 (1) This "mayBreak" vs. "aCanBreak" distinction is confusing.
 (2) It looks like aCanBreak *always* includes !CanContinueTextRun() -- and in 2/3 callers, it's exactly !CanContinueTextRun().  So it's a bit odd that this particular condition is the caller's responsibility, whereas all of these other conditions (e.g. StyleContext()->ShouldSuppressLineBreak()) is this function's responsibility.  Since the CanContinueTextRun() check is common to all callers, it seems like it should move into this list of common checks here.

I'd suggest the following changes to address these concerns:
 - Change this API take aFrame instead of aParentFrame (which makes slightly more sense anyway, for a DefaultAddInlineMinISize method).

 - Change aCanBreak to "aCallerAllowsBreaking" or something like that, and move it be the last parameter, so that we can give it a default value of "true" (so most callers don't need to worry about it). So then it *only* contains caller-specific conditions (if there are any such conditions).

 - Move !CanContinueTextRun() into the list of mayBreak conditions (and just restore the original "canBreak" to avoid unnecessary renaming from the original code, unless you really prefer "may" here):

     const bool mayBreak = aCallerAllowsBreaking &&
       !aFrame->CanContinueTextRun() &&
       parentFrame->[...] &&
       parentFrame->[...];

Does that make sense?

::: layout/generic/nsImageFrame.cpp
@@ +2402,5 @@
> +  nscoord isize = nsLayoutUtils::IntrinsicForContainer(aRenderingContext,
> +                    this, nsLayoutUtils::MIN_ISIZE);
> +  bool canBreak = !CanContinueTextRun() &&
> +                  !IsInAutoWidthTableCellForQuirk(this);
> +  aData->DefaultAddInlineMinISize(GetParent(), canBreak, isize);

If you agree with my comment about changing this API (noted above for nsFrame.cpp), then this callsite should now look like:

  bool canBreak = !IsInAutoWidthTableCellForQuirk(this);
  aData->DefaultAddInlineMinISize(this, isize, canBreak);

(The main point is, the CanContinueTextRun() check should happen inside the common code.)

        Attachment #8740754 -
        Flags: review?(dholbert) → review+

    
    

    
  
(thanks for doing that refactoring, BTW -- I hadn't realized nsImageFrame would also benefit from it.  That's a happy bonus, from a maintainability & code-understandability perspective!)

    
    

    
  
> So it's a bit odd that this particular condition is the caller's responsibility

I did that for two reasons:
1. to keep all preconditions on the frame itself on the caller side for clarity
2. it seems plausible that some classes wouldn't need to call CanContinueTextRun();
   neither nsBulletFrame, nor nsImageFrame does, AFAICT, because they inherit it
   from nsFrame which just does "return false".  Granted, nsImageFrame isn't
   final, so there's no guarantee there.

I don't feel strongly about it either way though, so I landed what you suggested.

(Besides, CanContinueTextRun seems like rather weird method to use in this
context anyway, why should that matter for where breaks can occur?
It seems wrong that it was generalized into this role too (beyond what its
name suggests).)

    
  
Flags: in-testsuite+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: