886313 - Text is clipped on previous lines by text-overflow: ellipsis

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Rob Crowther]

Attachment: overflow-ellipsis.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130511120803

Steps to reproduce:

If text-overflow: ellipsis is applied to multi-line text, all lines of text get clipped at the left edge.



Expected results:

Only the last line, where the ellipsis is, should be clipped.

Comment 1

[Rob Crowther]

Attachment: Screenshot of attachment 766696 in ESR showing correct behaviour

Comment 2

[Rob Crowther]

Attachment: Screenshot of attachment 766696 in nightly showing incorrect behaviour

Comment 3

[Rob Crowther]

Sorry, in my initial comment I should have said 'right edge' - always had trouble with that sort of thing.

An additional but more complex example (taken directly from code in my web app where I came across this) is available here: http://fiddle.jshell.net/robertc/EY8wX/show/

It's more obvious in the complex example because the text is right aligned.

Comment 4

[Loic]

Not sure if it's a valid regression, anyway:

good=2012-11-28
bad=2012-11-29
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c3a8eed0578&tochange=58a4e9501f7e

Comment 5

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b49f84c2c111
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121127 Firefox/20.0 ID:20121127235132
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7d678d658159
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121127 Firefox/20.0 ID:20121128010332
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b49f84c2c111&tochange=7d678d658159

Regressed by:
8ecf5ea9461d	Daniel Holbert — Bug 815972: Remove unneeded nsDisplayListCollection from nsBlockFrame.cpp's DisplayLine() helper-function. r=roc

Comment 6

[Daniel Holbert [:dholbert]]

Confirmed regression from that changeset.

On my linux machine, a build from 8ecf5ea9461d has the final "h" in "which" clipped, whereas a build with that cset backed out will correctly display the "h".

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: possible fix: backout bug 815972

Haven't dug into this enough yet to fully understand what's going on, but this patch (an unbitrotted backout of bug 815972) seems to fix things, as one would hope.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 766925 [details] [diff] [review]
possible fix: backout bug 815972

>   if (lineMayHaveTextOverflow) {
>-    aTextOverflow->ProcessLine(aLists, aLine.get());
>+    aTextOverflow->ProcessLine(collection, aLine.get());
>+  }

This is the key part of the backout that fixes things -- doing "aTextOverflow->ProcessLine()" for *just* the current line's display items, as opposed to all previous lines' display items.

In particular, this chunk at the end of ProcessLine is relevant (particularly the comment):
> 613   // Clip and remove display items as needed at the final marker edges.
> 614   nsDisplayList* lists[] = { aLists.Content(), aLists.PositionedDescendants() };
> 615   for (uint32_t i = 0; i < ArrayLength(lists); ++i) {
> 616     PruneDisplayListContents(lists[i], framesToHide, insideMarkersArea);
> 617   }
http://mxr.mozilla.org/mozilla-central/source/layout/generic/TextOverflow.cpp#613

So, 'collection' was needed after all, and we should add it back (with a comment explaining this).

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v1

(This is slightly cleaner than the direct backout. It preserves the local variable 'flags' that we added in bug 815972, to avoid needlessly re-checking a bool on each iteration of a loop.)

So: this patch just makes DisplayLine store its items in a temporary list "collection", and then moves collection's items to aLists just before we return. (as we used to do, before bug 815972)

I've included a reftest, which fails pre-patch and passes post-patch.

Comment 10

[Daniel Holbert [:dholbert]]

Here's a try push with just the reftest, to demonstrate that the test fails without the fix:
 https://tbpl.mozilla.org/?tree=Try&rev=051a3a693f39

...and here's a try push with the fix alongside the reftest:
 https://tbpl.mozilla.org/?tree=Try&rev=196cbb81292c

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 768534 [details] [diff] [review]
fix v1

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

Oops

Comment 12

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e67b73e7e1a

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2e67b73e7e1a