Last Comment Bug 672944 - text-overflow ellipsis with -moz-box
: text-overflow ellipsis with -moz-box
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla12
Assigned To: Mats Palmgren (:mats)
:
Mentors:
: 702207 (view as bug list)
Depends on:
Blocks: 241197
  Show dependency treegraph
 
Reported: 2011-07-20 14:05 PDT by Bryan Clark (DevTools PM) [@clarkbw]
Modified: 2012-01-28 18:57 PST (History)
9 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
box and text-overflow ellipsis test case (4.39 KB, text/html)
2011-07-20 14:05 PDT, Bryan Clark (DevTools PM) [@clarkbw]
no flags Details
screenshot of google chrome canary (23.43 KB, image/png)
2011-07-20 14:07 PDT, Bryan Clark (DevTools PM) [@clarkbw]
no flags Details
Proof-of-concept + mochitest (3.93 KB, patch)
2012-01-01 13:17 PST, Jonathan Protzenko [:protz]
mats: review-
Details | Diff | Splinter Review
Testcase #2 (634 bytes, text/html)
2012-01-02 03:26 PST, Mats Palmgren (:mats)
no flags Details
frame dump of Testcase #2 (5.23 KB, text/plain)
2012-01-02 03:27 PST, Mats Palmgren (:mats)
no flags Details
fix (11.53 KB, patch)
2012-01-24 12:10 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
reftest (7.01 KB, patch)
2012-01-24 12:11 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
reftest (7.00 KB, patch)
2012-01-26 15:40 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review

Description Bryan Clark (DevTools PM) [@clarkbw] 2011-07-20 14:05:49 PDT
Created attachment 547233 [details]
box and text-overflow ellipsis test case

The text-overflow: ellipsis seems to behave incorrectly within an element of display:-moz-box; hopefully this is a real bug.

I'm attaching a test html page to show the differences and will have compare screenshots with (chrome) webkit.

Each item has the class "bacon" which gives the following properties:

    .bacon {
      white-space:    nowrap;
      text-overflow:  ellipsis;
      overflow:       hidden;
      width:          400px;
    }

The first set is variations of wrapping a block with a box element.

1: display:block;
  ellipsis happens correctly here in both webkit and gecko

2: display:-moz-box;
  ellipsis happens correctly in webkit (it likely assumes a block display as it ignores -moz prefixes).  in gecko the block seems to ignore the width property which could be another unrelated bug.

3: display:-webkit-box;
  ellipsis happens correctly in webkit and in gecko (which likely assumes a block as it ignores -webkit prefixes)

4: display:box;
  ellipsis happens correctly as both webkit and gecko likely assume a block display

The second set is using a box directly as the text content parent.

1: display:block;
  ellipsis happens correctly here in both webkit and gecko

2: display:-moz-box;
  ellipsis happens correctly in webkit (it likely assumes a block display as it ignores -moz prefixes).  in gecko the text is truncated but without the ellipsis

3: display:-webkit-box;
  ellipsis happens correctly in gecko (which likely assumes a block as it ignores -webkit prefixes) in webkit the text is truncated but without the ellipsis

4: display:box;
  ellipsis happens correctly as both webkit and gecko likely assume a block display
Comment 1 Bryan Clark (DevTools PM) [@clarkbw] 2011-07-20 14:07:24 PDT
Created attachment 547235 [details]
screenshot of google chrome canary

This is a screenshot of google chrome canary, the rendering is the same in google chrome stable.

Just to note that the Firefox test was done with nightly version: 8.0a1 (2011-07-20)
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-07-20 15:36:38 PDT
I think this has rather little to do with text-overflow:ellipsis and a lot to do with the sizes that the box code makes things.
Comment 3 Jonathan Protzenko [:protz] 2011-08-23 16:41:09 PDT
Ha! I just ran into this while trying to use text-overflow: ellipsis. The second line is the one that I'd like to see triggering text-overflow (obviously).

I also tried constraining the inner .bacon with top: 0 left: 0 right: 0 bottom: 0 so that it would be exactly as large as the size allocated to its parent box, but that didn't seem to help. My testcase: http://jonathan.protzenko.free.fr/css/mozbox.html
Comment 4 John P Baker 2011-11-14 06:11:08 PST
*** Bug 702207 has been marked as a duplicate of this bug. ***
Comment 5 Jonas H. 2011-12-26 15:00:27 PST
How can I help to get this fixed? (besides coding, which I'm not into)
Comment 6 Jonathan Protzenko [:protz] 2011-12-28 13:15:29 PST
Mats, could you possibly give a rough outline of what to do in order to fix this? I have some time on my hands during the holiday season, and if this is within reach, I might as well try to fix it. (I suppose you know how to do this since you implemented text-overflow: ellipsis in the first place.)

Thanks :)
Comment 7 Jonathan Protzenko [:protz] 2012-01-01 12:47:36 PST
So after looking into the problem, here's what I've been able to gather so far.

- The problem happens when we ask the outer block frame "what's your min width?". The block frame goes on asking its lines to advertise how much min width they want: this is (if I'm not mistaken) nsBlockFrame.cpp:734

          kid->AddInlineMinWidth(aRenderingContext, &data);

- The underlying text frame then complies with the request, and the min width is bumped in nsTextFrameThebes.cpp:6536

      aData->currentLine = NSCoordSaturatingAdd(aData->currentLine, width);

Skipping that line "solves" the problem.

- My intuition is that we should be smarter in nsBlockFrame::GetMinWidth. That is, if the block frame has text-overflow: clip or ellipsis, this means that its min width is zero, because we can hide as much text as needed. Thus, we should not even proceed with the call to AddInlineMinWidth if we're in a situation of text-overflow.

- I don't want to duplicate the whole logic that decides whether TextOverflow should be effective. However, it seems I can't reuse the logic from TextOverflow::CanHaveTextOverflow, because that function is written to be used from DisplayBuildList, not in the context of GetMinWidth.

I'll see if I can hack something up :).
Comment 8 Jonathan Protzenko [:protz] 2012-01-01 13:17:32 PST
Created attachment 585221 [details] [diff] [review]
Proof-of-concept + mochitest

Hi Mats,

I'd be happy to hear your thoughts on this. Feel free to redirect the review if you're not the right person!

The patch isn't worth much, but I could use some guidance on how to do this properly.

Thanks,

jonathan
Comment 9 Mats Palmgren (:mats) 2012-01-02 03:25:27 PST
Comment on attachment 585221 [details] [diff] [review]
Proof-of-concept + mochitest

I don't think we should mess with min/pref sizes or anything that
affects layout.
Comment 10 Mats Palmgren (:mats) 2012-01-02 03:26:25 PST
Created attachment 585271 [details]
Testcase #2
Comment 11 Mats Palmgren (:mats) 2012-01-02 03:27:57 PST
Created attachment 585272 [details]
frame dump of Testcase #2

I think the root of the problem is that the nsXULScrollFrame child
frame doesn't have a sane scrollable overflow area.  One approach
that might work is to treat the -moz-xul-anonymous-block pseudo frame
specially by using its ancestor scroll frame when calculating the clip
area.  As you can see it doesn't have any scrollable oveflow, nor does
its lines, so you need to force your way past those checks in the
TextOverflow code.
Comment 12 Mats Palmgren (:mats) 2012-01-02 03:33:38 PST
... and by "calculating the clip area" I mean TextOverflow::mContentArea here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/TextOverflow.cpp#262
that's the area that's used for ellipsing.
Comment 13 Mats Palmgren (:mats) 2012-01-02 03:38:47 PST
roc, do you see a better solution?
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-02 03:56:23 PST
(In reply to Mats Palmgren [:mats] from comment #11)
> I think the root of the problem is that the nsXULScrollFrame child
> frame doesn't have a sane scrollable overflow area.

Seems like that should be fixed directly.
Comment 15 Jonathan Protzenko [:protz] 2012-01-02 05:22:53 PST
Sure, I'll try to do that. But what's the intuition with GetMinWidth? If it doesn't really return a "minimum width", what is the function's goal exactly?
Comment 16 Jonathan Protzenko [:protz] 2012-01-02 05:24:34 PST
Also, Mats, how did you get the frame dump of only the HTML contents? Whenever I use -layoutdebug and hit Dump > Frames, it dumps the frames for the entire window, including its chrome, and not just the html content area.
Comment 17 Mats Palmgren (:mats) 2012-01-02 05:45:55 PST
Start Firefox normally, then Tools->Layout Debugger.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-02 13:30:55 PST
(In reply to Jonathan Protzenko [:protz] from comment #15)
> Sure, I'll try to do that. But what's the intuition with GetMinWidth? If it
> doesn't really return a "minimum width", what is the function's goal exactly?

It returns the intrinsic minimum width, which is the width required to lay out the element without overflowing its container if you assume that all optional line breaks are taken.
Comment 19 Jonathan Protzenko [:protz] 2012-01-21 12:58:25 PST
Huh I'm confused because the Box frame that's right under the XULScroll frame does have the "right" scrollable area. (I'm assuming that's what you referred to when you wrote "the nsXULScrollFrame child frame doesn't have a sane scrollable overflow area".)

(gdb) x/wa *({void**}0x7ffff6d04820)
0x7ffff263b78c <nsXULScrollFrame::QueryFrame(nsQueryFrame::FrameIID)>:	0xffffffffe5894855

(gdb) p ({nsXULScrollFrame}0x7ffff6d04820)->mInner.mScrolledFrame->GetScrollableOverflowRect()
[Thread 0x7fffd05c9700 (LWP 20217) exited]
$42 = {<mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>> = {
    x = 0, y = 0, width = 29460, height = 1140}, <No data fields>}

However, its content area is indeed too big:

(gdb) p ({nsXULScrollFrame}0x7ffff6d04820)->mInner.mScrolledFrame->mRect
$43 = {<mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>> = {
    x = 0, y = 0, width = 29460, height = 1140}, <No data fields>}

That's as far as I could get, I'll try to figure out why the nsBoxFrame insists on having 24960 units as its width while the stylesheet explicitly requests 24000. It may be the nsXULScrollFrame allocates too much width for its child, or the nsBoxFrame ignores the stylesheet (I guess...).
Comment 20 Mats Palmgren (:mats) 2012-01-21 14:52:00 PST
GetScrollableOverflowRect() just returns the frame rect when there's no registered
overflow areas, see the last line in nsIFrame::GetOverflowRect.  (and in that case
the "scr-overflow=" is omitted in the frame dump)

(gdb) p this->mInner.mScrolledFrame->HasOverflowAreas()
$1 = false

FYI, you can use aFrame->List(stdout,0) in gdb to get a frame dump.

> why the nsBoxFrame insists on having 24960 units as its width while the stylesheet
> explicitly requests 24000

That's 16px, almost certainly the width of a scrollbar.
Hmm, did you get that with the overflow:hidden testcase?
Anyway, that's not important for this bug I think.
Comment 21 Mats Palmgren (:mats) 2012-01-24 12:10:41 PST
Created attachment 591211 [details] [diff] [review]
fix

> layout/generic/nsGfxScrollFrame.cpp
Make nsXULScrollFrame report overflow for the relevant frame.

> layout/generic/TextOverflow.*
Move initialisation code to a separate method, TextOverflow::Init.
Add code for mozXULAnonymousBlock that get the scroll frame from the parent.
Do the 1px tweak in this case also, to counter rounding of scroll positions
by nsXULScrollFrame.
Comment 22 Mats Palmgren (:mats) 2012-01-24 12:11:20 PST
Created attachment 591212 [details] [diff] [review]
reftest
Comment 23 Mats Palmgren (:mats) 2012-01-24 12:15:33 PST
Jonathan, can you try the attached patch and see if it works for the feature
you're working on?  Thanks.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-24 12:31:26 PST
Comment on attachment 591212 [details] [diff] [review]
reftest

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

::: layout/reftests/text-overflow/reftest.list
@@ +20,5 @@
>  HTTP(..) == table-cell.html table-cell-ref.html
>  HTTP(..) == two-value-syntax.html two-value-syntax-ref.html
>  HTTP(..) == single-value.html single-value-ref.html 
>  HTTP(..) == atomic-under-marker.html atomic-under-marker-ref.html
> +fuzzy(1,250000) skip-if(Android) HTTP(..) == xulscroll.html xulscroll-ref.html

Er, why so many fuzzy pixels?
Comment 25 Jonathan Protzenko [:protz] 2012-01-25 02:16:37 PST
So your patch works just fine for me, unfortunately, it isn't enough for me to achieve what I want. I've put online another testcase at http://jonathan.protzenko.free.fr/bug672944.html : I wish the text-overflow: ellipsis would kick in for the second lime box, instead of making the outer box (.wrapper) larger than 400px. However, reading http://www.w3.org/TR/css3-flexbox/, I'm having a hard time figuring out what is the "correct" behavior. I'm not even sure what I want is possible with the current flex box model...
Comment 26 Mats Palmgren (:mats) 2012-01-25 06:00:48 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
>> +fuzzy(1,250000) skip-if(Android) HTTP(..) == xulscroll.html xulscroll-ref.html
> Er, why so many fuzzy pixels?

As long as they only differ by 1 in color it's due to anti-aliasing and I don't
really care how many AA pixels there are.
Comment 27 Mats Palmgren (:mats) 2012-01-25 06:16:50 PST
(In reply to Jonathan Protzenko [:protz] from comment #25)
> So your patch works just fine for me, unfortunately, it isn't enough for me
> to achieve what I want. I've put online another testcase at
> http://jonathan.protzenko.free.fr/bug672944.html : I wish the text-overflow:
> ellipsis would kick in for the second lime box, instead of making the outer
> box (.wrapper) larger than 400px. 

Please note that 'text-overflow' does not affect layout in any way (that is,
sizes and positions of boxes), it's applied after layout.

Maybe what you want is:
  .wrapper > * {
    -moz-box-flex: 1;
    display: -moz-box;
  }

> However, reading
> http://www.w3.org/TR/css3-flexbox/, I'm having a hard time figuring out what
> is the "correct" behavior. I'm not even sure what I want is possible with
> the current flex box model...

-moz-box pre-dates the css3-flexbox spec by many years so you shouldn't
expect that it matches what the spec says (they are quite unrelated).
We are working on implementing css3-flexbox in bug 666041.
Comment 28 Jonathan Protzenko [:protz] 2012-01-25 09:45:25 PST
Oh thanks, right, I'd forgotten, this is exactly what I needed and the bug precisely fixes this. Sorry for the noise, fixing this bug indeed solves my very issue :).
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-25 20:23:50 PST
(In reply to Mats Palmgren [:mats] from comment #26)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> >> +fuzzy(1,250000) skip-if(Android) HTTP(..) == xulscroll.html xulscroll-ref.html
> > Er, why so many fuzzy pixels?
> As long as they only differ by 1 in color it's due to anti-aliasing and I
> don't really care how many AA pixels there are.

Can you put the correct number in there anyway?
Comment 30 Mats Palmgren (:mats) 2012-01-26 15:40:52 PST
Created attachment 591971 [details] [diff] [review]
reftest

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