Mats Palmgren (inactive) Assignee
	Description • 13 years ago

Follow-up from bug 312156.

Text under the text-overflow marker (ellipsis) is displayed (overlaps) when text is selected.

The problem is that the paint-with-selection path needs to know about
the character clipping already done by nsTextFrame::PaintText:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#5267

Basically just propagate 'startOffset' and 'maxLength' to
PaintTextWithSelection etc...

	David Baron :dbaron:
	Comment 1 • 13 years ago

I think this *may* be bad enough that we shouldn't ship text-overflow with it, so nominating for tracking-firefox7.

	Dão Gottwald [:dao]
	Comment 2 • 13 years ago

(In reply to comment #1)
> I think this *may* be bad enough that we shouldn't ship text-overflow with
> it, so nominating for tracking-firefox7.

Considering how badly wanted text-overflow:ellipsis is and that Gecko is the only engine lacking it, I think the bar needs to be a little higher than that. As I understand it, fixing this glitch in a later release would be backwards compatible, so this shouldn't prevent people from finally using this feature.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 3 • 13 years ago

It really is bad. I didn't realize it was going to be this bad. But I'm confident Mats can fix it before we branch :-)

	Tom Lee
	Comment 4 • 13 years ago

Here's a patch implementing the clipping behaviour with the brief guidance Mats gave in the earlier comment. From a quick test it seems to work fine, but I'm very new to the code base and especially new to the renderer code.

I don't have enough understanding of the code to see how the startOffset var that Mats mentioned is relevant to this change -- I only wound up using maxLength.

The patch is simple enough, so if somebody could take a look at nudge me in the right direction it'd be much appreciated!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 5 • 13 years ago

You definitely need to use the offset. Imagine you have a scrollable DIV that's scrolled to the right a bit. We'll chop off text on the left to make room for the ellipsis, and the offset will be greater than zero. Text before the offset should not be drawn.

	Patrick Walton (:pcwalton)
	Comment 6 • 13 years ago

(In reply to comment #1)
> I think this *may* be bad enough that we shouldn't ship text-overflow with
> it, so nominating for tracking-firefox7.

In addition to Dao's comment, please note that I would like to use text-overflow in chrome UI (bug 666842). The text in question is not selectable, so this bug doesn't apply.

	Mats Palmgren (inactive) Assignee
	Comment 7 • 13 years ago

Comment on attachment 541850 [details] [diff] [review]
Initial Patch

What roc said, and you need to adjust the horizontal coordinate where
the chopped text starts;  selection decorations needs fixing too
(e.g. that red wavy underline for misspelled words in a <textarea>).

	Mats Palmgren (inactive) Assignee
	Comment 10 • 13 years ago

There is also a "part 3" but I spawned that off as bug 667653,
since it orthogonal to this bug.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 12 • 13 years ago

Comment on attachment 542358 [details] [diff] [review]
Part 1, clip selected text, background and selection decorations

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

::: layout/generic/nsTextFrameThebes.cpp
@@ +4665,5 @@
>  class SelectionIterator {
>  public:
>    /**
>     * aStart and aLength are in the original string. aSelectionDetails is
>     * according to the original string.

Document aXOffset

@@ +4960,5 @@
>    if (aProvider.GetFontGroup()->ShouldSkipDrawing())
>      return;
>  
> +  PRInt32 contentOffset = aProvider.GetStart().GetOriginalOffset() + aStartDelta;
> +  PRInt32 contentLength = aProvider.GetOriginalLength() - aLengthDelta;

Are aStartDelta/aLengthDelta in content offsets or "skipped" offsets? Here you're assuming content offsets, but the calculation in PaintText computes skipped offsets.

You'll probably want to fix this by using a gfxSkipCharsIterator to calculate the values in content offsets in PaintText.

I guess we'll need an additional testcase for this.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 13 • 13 years ago

Comment on attachment 542359 [details] [diff] [review]
Part 2, tests

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

	Mats Palmgren (inactive) Assignee
	Comment 14 • 13 years ago

BTW, there's kind of a funny effect when you select some text in a
text input/area with a text-overflow marker in it.  The mousedown
(or focus rather) causes the text caret to appear and thus the marker
is inhibited and you see the full text... then as you start dragging
with the mouse pressed (to select text) the caret is inhibited (default
behavior when there is a selection) so now the text-overflow marker
appears again... so if you drag the mouse back again over the point
where you started the selection becomes collapsed -> caret shows again
-> marker disappears.

Here's test builds if you want to try it out:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-5b011dbb6303/

	Mats Palmgren (inactive) Assignee
	Comment 15 • 13 years ago

(In reply to comment #12)
> Here
> you're assuming content offsets, but the calculation in PaintText computes
> skipped offsets.

Ah, right.

> I guess we'll need an additional testcase for this.

Do you know an example text that would fail?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 16 • 13 years ago

(In reply to comment #14)
> BTW, there's kind of a funny effect when you select some text in a
> text input/area with a text-overflow marker in it.  The mousedown
> (or focus rather) causes the text caret to appear and thus the marker
> is inhibited and you see the full text... then as you start dragging
> with the mouse pressed (to select text) the caret is inhibited (default
> behavior when there is a selection) so now the text-overflow marker
> appears again... so if you drag the mouse back again over the point
> where you started the selection becomes collapsed -> caret shows again
> -> marker disappears.

Seems like the best behavior would be to suppress the marker if the caret *or* any selection would overlap the marker, and not suppress it otherwise.

That would be useful for find-as-you-type as well, so we don't hide find matches behind the marker.

(In reply to comment #15)
> Do you know an example text that would fail?

Anything with whitespace compression. Say
<div style="overflow:scroll; text-overflow:hidden">A        B blah blah blah blah</div>
and then scroll right a very small amount. With a monospaced font I think you'd get an ellipsis on the left that should cover "A B", so the skipped-offsets delta is 3, but we'll actually draw the B since B's content offset is > 3.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 13 years ago

Well, that's if you're using three dots instead of an ellipsis.

	Mats Palmgren (inactive) Assignee
	Comment 18 • 13 years ago

Use ConvertSkippedToOriginal() to convert from transformed offsets to
original content offsets, and pass that to the paint selection methods.

	Mats Palmgren (inactive) Assignee
	Comment 19 • 13 years ago

Add a couple of tests where transformed != original offsets, that fails
with the previous patch revision.

BTW, I filed bug 668240 about improving how selection/caret affects the
text-overflow markers.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 20 • 13 years ago

Comment on attachment 542846 [details] [diff] [review]
Part 1, clip selected text, background and selection decorations

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 21 • 13 years ago

Comment on attachment 542848 [details] [diff] [review]
Part 2, tests

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

	Mats Palmgren (inactive) Assignee
	Comment 22 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/e6f9648715c1
http://hg.mozilla.org/integration/mozilla-inbound/rev/80ecb84cc680

	Marco Bonardo [:mak]
	Comment 23 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/e6f9648715c1
http://hg.mozilla.org/mozilla-central/rev/80ecb84cc680