Closed Bug 1463745 Opened 6 years ago Closed 6 years ago

The Text around shape-outside does not fill the empty space [shape-outside]

Categories

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

Product:
Core
Component:
Layout: Floats
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 + verified

People

(Reporter: rdoghi, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(9 files)

polygon-shape-collapse.mp4
4.95 MB, video/mp4
Details
ShapesFill.png
909.22 KB, image/png
Details
FilledSPACEshape.png
909.58 KB, image/png
Details
Grapes.png
948.52 KB, image/png
Details
tapered-float-areas.html
1.22 KB, text/html
Details
Bug 1463745 Part 1: Add methods to FloatInfo and ShapeInfo to report whether or not they could narrow in the block direction.
59 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1463745 Part 2: Change nsFlowAreaRect to also track whether it may widen in the block direction.
59 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1463745 Part 3: Change nsBlockFrame::DoReflowInlineFrames to detect when the float area may widen in the block direction, and push the inline frame down by a pixel at a time to retry until it fits.
59 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1463745 Part 4: Add tests of inline elements being pushed in block direction past too-wide sections of shape-outside floats.
59 bytes, text/x-review-board-request
dbaron
: review+
Details 
      No description provided.
[Affected versions]: 
Nightly 62.0a1

[Affected platforms]:
Platforms: Windows 10 x 64, Windows 7 x 32, Mac OS X 10.12 and Ubuntu 16.04 x64.

[Steps to reproduce]:
1. Go to about:config and set "layout.css.shape-outside.enabled" to true.
2. Go to http://labs.jensimmons.com/2016/examples/shapes-3.html
3. Right click on the image and then click on "Inspect Element".
4. Click the button next to Shape-Outside in order to turn the highlighter on.
5. Move The highlighter points from the Bottom Right all the way to the LEFT side of the photo.
6. Move one of the highlighter points from the top right a bit more to the right.


[Expected result]:
- The Text should wrap around the highlighter and fill the empty space available.

[Actual result]:
- The Text will move under the Picture regardless of the available empty space.

Please check the attachments for more information.

    
  
Blocks: shape-outside

    
    

    
  

      
      
      
      

        Attached image
          
        ShapesFill.png
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached image
          
        FilledSPACEshape.png
        — 
      

    


  

    
  
Assignee: nobody → bwerth

    
    

    
  
I'm sorry, but I'm just not able to reproduce this, either by replicating the 5-point polygon in the video or by moving the points to a similar position in the ShapesFill.png. When you have a polygon that shows the defect, would you please copy the new polygon definition from the devtools Rule View and paste it here?
Flags: needinfo?(rares.doghi)
Priority: -- → P3

    
    

    
  
Hi everyone, here is the definition for the shape outside :

polygon(14.82% 71.97%, 23.21% 90.6%, 0% 100%, 0% 88.4%, 0% 23.7%, 17.72% 21.4%, 37.33% 14.8%, 54.27% 17.38%, 68.13% 21.64%, 77.69% 28.97%, 71.17% 32.18%, 57.08% 35.67%, 48.56% 37.49%, 41.56% 41.74%, 24.79% 53.33%, 21.38% 57.18%)


it really just needs to be 1 highlighter point from above all the way to the right in order for the text to move under the image instead of under the highlighter.
Flags: needinfo?(rares.doghi)

    
    

    
  

      
      
      
      

        Attached image
          
        Grapes.png
        — 
      

    


  

    
    

    
  
I'm able to reproduce it, but only if my viewport width is 1800px or higher. Below everything seems fine.

    
    

    
  
Thank you for the reproduction polygon. With the very wide viewport, I can reproduce this also. The point of failure comes when the float area of the polygon is so wide relative to the text column that not even one word can fit in the line (the word "proposed", in my reproduction). When that happens, the word that failed to fit is moved all the way down below the bottom-most point of the float area. This is very likely an optimization we've built that is based on the assumption that all float areas have flat sides, and so if a word doesn't fit at any point, it won't fit at any further point either. I'll look for that code.

    
    

    
  


  
This reduced testcase shows that text flowing around a tapered float area will "give up" when encountering a word that can't fit in a line, and bump that word in the block axis flow direction beyond the entire flow area.

    
    

    
  
Tracking for 62, as this may affect whether we ship the feature (Dev Tools Shapes editor) in 62.

          tracking-firefox62:
          --- → +

    
    

    
  
David, I'd appreciate your insights here because I'm not sure I can understand the existing behavior quickly enough without your guidance. It appears that somewhere in nsBlockFrame::ReflowBlockFrame() we check to see if there's enough clearance for a frame, and if there isn't, we clear the whole float. To handle this case correctly for floats from shape-outside, we instead need to continue to check the clearance in the block direction one block line down. So I would greatly appreciate your help with understanding...

1) Where in that function do we make the final call that the frame won't fit?
2) What's an example of advancing the blockreflowinput by one block line -- similar I suppose to how a br tag is handled?
Flags: needinfo?(dbaron)

    
    

    
  
(In reply to Brad Werth [:bradwerth] from comment #12)
> David, I'd appreciate your insights here because I'm not sure I can
> understand the existing behavior quickly enough without your guidance. It
> appears that somewhere in nsBlockFrame::ReflowBlockFrame() we check to see
> if there's enough clearance for a frame, and if there isn't, we clear the
> whole float. To handle this case correctly for floats from shape-outside, we
> instead need to continue to check the clearance in the block direction one
> block line down. So I would greatly appreciate your help with
> understanding...

Quick but hopefully usable answer since I have a meeting in one minute...

> 
> 1) Where in that function do we make the final call that the frame won't fit?

The code you should be looking for is the code that handles LineReflowStatus::RedoNextBand.  (See the documentation of the enum in nsBlockFrame.h and the code that handles it in the various interacting functions in nsBlockFrame.cpp.)

> 2) What's an example of advancing the blockreflowinput by one block line --
> similar I suppose to how a br tag is handled?

I think you should be thinking one pixel here, not one line.  But see previous answer.
Flags: needinfo?(dbaron)
Priority: P3 → P1

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8982069 -
        Flags: review?(dbaron)

        Attachment #8982070 -
        Flags: review?(dbaron)

        Attachment #8982071 -
        Flags: review?(dbaron)

        Attachment #8982072 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 8982069 [details]
Bug 1463745 Part 1: Add methods to FloatInfo and ShapeInfo to report whether or not they could narrow in the block direction.

https://reviewboard.mozilla.org/r/248108/#review255236

::: layout/generic/nsFloatManager.cpp:536
(Diff revision 2)
>    virtual nscoord BStart() const = 0;
>    virtual nscoord BEnd() const = 0;
>    virtual bool IsEmpty() const = 0;
>  
> +  // Does this shape possibly get inline narrower when proceeding in the
> +  // block direction? This is false for rectangles without rounded corners,

seems like it's false for rectangles without rounded corners *that extend to the bottom of the margin box*

::: layout/generic/nsFloatManager.cpp:1102
(Diff revision 2)
> +  bool MayNarrowInBlockDirection() const override {
> +    // Only possible to narrow if there are non-null mRadii.
> +    return !!mRadii;
> +  }

Why isn't this true for boxes that are inset at the bottom?

(You should have tests for this case, too!)

::: layout/generic/nsFloatManager.cpp:2539
(Diff revision 2)
> +  // We don't care about a block band, or whether we're checking left or
> +  // right. This is just a check if it's even possible for the FloatInfo
> +  // to narrow in the block direction (it isn't possible for rects).

It's not clear to me what this comment is trying to say.

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

    
    

    
  
Comment on attachment 8982070 [details]
Bug 1463745 Part 2: Change nsFlowAreaRect to also track whether it may widen in the block direction.

https://reviewboard.mozilla.org/r/248110/#review255238

This looks good, but I'd like to see the revised version with the flags rather than successive booleans.

::: layout/generic/nsFloatManager.h:46
(Diff revision 2)
> +  bool mMayWiden;
>  
>    nsFlowAreaRect(mozilla::WritingMode aWritingMode,
>                   nscoord aICoord, nscoord aBCoord,
>                   nscoord aISize, nscoord aBSize,
> -                 bool aHasFloats)
> +                 bool aHasFloats, bool aMayWiden)

It's probably better to turn this into a flags parameter that takes each thing as part of an enum class bitfield.  This makes things much clearer at the caller.

::: layout/generic/nsFloatManager.cpp:225
(Diff revision 2)
>            // probably provide better information at some point.
>            haveFloats = true;
> +
> +          // Our area may widen in the block direction if this float may
> +          // narrow in the block direction.
> +          mayWiden |= fi.MayNarrowInBlockDirection(aShapeType);

I think it's both clearer and probably faster (avoids the call in many cases) to write:

mayWiden = mayWiden || fi.MayNarrowInBlockDirection(aShapeType);

(same about 10 lines lower)

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

    
    

    
  
Comment on attachment 8982071 [details]
Bug 1463745 Part 3: Change nsBlockFrame::DoReflowInlineFrames to detect when the float area may widen in the block direction, and push the inline frame down by a pixel at a time to retry until it fits.

https://reviewboard.mozilla.org/r/248112/#review255240

::: layout/generic/nsBlockFrame.cpp:4057
(Diff revision 2)
> -      NS_ASSERTION(NS_UNCONSTRAINEDSIZE != aState.mReflowInput.AvailableBSize(),
> -                   "We shouldn't be running out of height here");
> +      if (NS_UNCONSTRAINEDSIZE == aState.mReflowInput.AvailableBSize() ||
> +          aFloatAvailableSpace.mMayWiden) {

This || doesn't make sense.  It seems like it will break cases where we do need to push the line to the next column or page, but we happen to be next to floats that can narrow.

I think you should instead restructure the if-part of the outer if (and not stick the && in its condition), rather than the else-part.

::: layout/generic/nsBlockFrame.cpp:4059
(Diff revision 2)
>          // just move it down a bit to try to get out of this mess
>          aState.mBCoord += 1;

This isn't a pixel at a time -- it's an appunit at a time, which is 1/60 of a pixel.

It's probably reasonable to change it to a pixel -- although maybe a device pixel rather than a CSS pixel.

This was previously an error handling case so this code wasn't particularly well exercised.

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

    
    

    
  
Comment on attachment 8982072 [details]
Bug 1463745 Part 4: Add tests of inline elements being pushed in block direction past too-wide sections of shape-outside floats.

https://reviewboard.mozilla.org/r/248114/#review255242

So I haven't reviewed these very closely yet, but three comments:

 1. they should really be contributed to the CSS testsuite, probably via layout/reftests/w3c-css/submitted/shapes1/ or alternatively via testing/web-platform/tests/css/css-shapes/, so they should have the metadata for that.
 2. please don't use "floater" for the inline-block.  I have managed to get rid of "floater" as a synonym for "float" in our source tree, but some others (particularly on the Edge team at Microsoft) still use it, so it's confusing to use "floater" as the id for the non-floating thing in the testcase.  Also probably prefer "shape" over "shaper".
 3. Have you looked at how other browsers do on these tests?  (I'm particularly interested in whether they find *slightly* different positions.)

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

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8982069 [details]
Bug 1463745 Part 1: Add methods to FloatInfo and ShapeInfo to report whether or not they could narrow in the block direction.

https://reviewboard.mozilla.org/r/248108/#review255236

> seems like it's false for rectangles without rounded corners *that extend to the bottom of the margin box*

Float layout is only concerned with the area between FloatInfo::BStart() and FloatInfo::BEnd(), so the rectangles only have to extend to BEnd(). I've updated the comment to note that.

> Why isn't this true for boxes that are inset at the bottom?
> 
> (You should have tests for this case, too!)

Insets adjust the mRect that the RoundedBoxShapeInfo is built around. mRect determines BStart() and BEnd(), which are the only regions that matter for the float shape. It won't get checked past BEnd(). I'll add tests.

> It's not clear to me what this comment is trying to say.

I've updated the comment.

    
    

    
  
Comment on attachment 8982072 [details]
Bug 1463745 Part 4: Add tests of inline elements being pushed in block direction past too-wide sections of shape-outside floats.

https://reviewboard.mozilla.org/r/248114/#review255242

I'll add these tests to the CSS testsuite, and make the naming changes you suggest also. These tests pass in Safari, and all but the image test passes in Chrome. Since the image test is based on a diagonal gradient, this is probably due to some difference in either gradient rendering (unlikely) or shape-outside: image interpretation of gradients. I may add an additional image test that uses a PNG triangle to disambiguate these possible reasons.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8982069 [details]
Bug 1463745 Part 1: Add methods to FloatInfo and ShapeInfo to report whether or not they could narrow in the block direction.

https://reviewboard.mozilla.org/r/248108/#review256640

r=dbaron with the following

::: layout/generic/nsFloatManager.cpp:537
(Diff revision 4)
>    virtual nscoord BEnd() const = 0;
>    virtual bool IsEmpty() const = 0;
>  
> +  // Does this shape possibly get inline narrower when proceeding in the
> +  // block direction? This is false for unrounded rectangles that span all
> +  // the way to BEnd(), but could be true for other shapes.

If it's the case, maybe clarify when BEnd() doesn't correspond to the block-end margin edge?  (It soundedlike that was the case from comment 33.)

        Attachment #8982069 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 8982070 [details]
Bug 1463745 Part 2: Change nsFlowAreaRect to also track whether it may widen in the block direction.

https://reviewboard.mozilla.org/r/248110/#review256650

OK, r=dbaron, although in the future it's better to use an enum class rather than a regular enum (probably with MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS().

        Attachment #8982070 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 8982071 [details]
Bug 1463745 Part 3: Change nsBlockFrame::DoReflowInlineFrames to detect when the float area may widen in the block direction, and push the inline frame down by a pixel at a time to retry until it fits.

https://reviewboard.mozilla.org/r/248112/#review256660

r=dbaron with the following comment tweak

::: layout/generic/nsBlockFrame.cpp:4039
(Diff revision 4)
> -    // then reflow the line all over again.
> +    // past the current band (if it's non-zero and the band definitely won't
> +    // widen), otherwise we try one pixel down. If there's no block space

I'd change "won't widen" to "won't widen around a shape-outside" so that it's clear to readers of the comment what feature this code is supporting.

        Attachment #8982071 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 8982072 [details]
Bug 1463745 Part 4: Add tests of inline elements being pushed in block direction past too-wide sections of shape-outside floats.

https://reviewboard.mozilla.org/r/248114/#review256668

This patch shouldn't require you to regenerate testing/web-platform/meta/MANIFEST.json .  On the other hand, maybe it needs regeneration -- but probably best not in this patch.

These tests need the metadata for CSS test submissions; see other tests in this directory.  (Note that the references don't need the rel="match" or the assert for what they're testing -- so references need a little less metadata than tests.)

review- because I'd like to look at the metadata additions, but otherwise the tests look good modulo the following comment:

::: layout/reftests/w3c-css/submitted/shapes1/float-retry-push-inset.html:21
(Diff revision 5)
> +    shape-outside: inset(0px 40px 40px 0px round 0 0 34.142135px 0);
> +    /* 34.142135 = 10 / (1 - (sqrt(2) / 2)) */

In order to ensure the box fits, maybe you should round the other way to 34.142136px?

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

    
    

    
  
Comment on attachment 8982070 [details]
Bug 1463745 Part 2: Change nsFlowAreaRect to also track whether it may widen in the block direction.

https://reviewboard.mozilla.org/r/248110/#review256650

I've updated the patch to use MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS, which didn't seem to work with a nested enum class, so I pulled the flag class out.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8982072 [details]
Bug 1463745 Part 4: Add tests of inline elements being pushed in block direction past too-wide sections of shape-outside floats.

https://reviewboard.mozilla.org/r/248114/#review256786

Please also add the "author" metadata to the reference file.

r=dbaron with that

        Attachment #8982072 -
        Flags: review?(dbaron) → review+

    
    

    
      
  

    
    

    
  
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66fea2d73825
Part 1: Add methods to FloatInfo and ShapeInfo to report whether or not they could narrow in the block direction. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/0aa1a48450be
Part 2: Change nsFlowAreaRect to also track whether it may widen in the block direction. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/411cd226acf6
Part 3: Change nsBlockFrame::DoReflowInlineFrames to detect when the float area may widen in the block direction, and push the inline frame down by a pixel at a time to retry until it fits. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/a89be4611fd9
Part 4: Add tests of inline elements being pushed in block direction past too-wide sections of shape-outside floats. r=dbaron

    
    

    
  
Hi, i retested this issue and the problem no longer occurs , i tested this issue on Mac OsX, Ubuntu and Windows 10 using the latest version of nightly 62.0a1 (2018-06-10)
Status: RESOLVED → VERIFIED

          status-firefox62:
          fixed → verified

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: