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

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

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...

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[Dão Gottwald [:dao]]

(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.

Comment 3

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

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 :-)

Comment 4

[Tom Lee]

Attachment: Initial Patch

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!

Comment 5

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

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.

Comment 6

[Patrick Walton (:pcwalton)]

(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.

Comment 7

[Mats Palmgren (inactive)]

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>).

Comment 8

[Mats Palmgren (inactive)]

Attachment: Part 1, clip selected text, background and selection decorations

Comment 9

[Mats Palmgren (inactive)]

Attachment: Part 2, tests

Comment 10

[Mats Palmgren (inactive)]

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

Comment 12

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

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.

Comment 13

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

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

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

Comment 14

[Mats Palmgren (inactive)]

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/[e-mail]-5b011dbb6303/

Comment 15

[Mats Palmgren (inactive)]

(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?

Comment 16

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

(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.

Comment 17

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

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

Comment 18

[Mats Palmgren (inactive)]

Attachment: Part 1, clip selected text, background and selection decorations

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

Comment 19

[Mats Palmgren (inactive)]

Attachment: Part 2, tests

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.

Comment 20

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

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

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

Comment 21

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

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

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

Comment 22

[Mats Palmgren (inactive)]

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

Comment 23

[Marco Bonardo [:mak]]

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