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

RESOLVED FIXED in mozilla7

Status

()

Core
Layout: Block and Inline
--
minor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Mats Palmgren (vacation - back in August), Assigned: Mats Palmgren (vacation - back in August))

Tracking

({css3})

Trunk
mozilla7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 3 obsolete attachments)

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...
Blocks: 666696
I think this *may* be bad enough that we shouldn't ship text-overflow with it, so nominating for tracking-firefox7.
tracking-firefox7: --- → ?
(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.
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

6 years ago
Created attachment 541850 [details] [diff] [review]
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!
Attachment #541850 - Flags: review?(matspal)
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.
(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.
Blocks: 667653
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>).
Attachment #541850 - Flags: review?(matspal) → review-
Created attachment 542358 [details] [diff] [review]
Part 1, clip selected text, background and selection decorations
Attachment #541850 - Attachment is obsolete: true
Attachment #542358 - Flags: review?
Created attachment 542359 [details] [diff] [review]
Part 2, tests
Attachment #542359 - Flags: review?(roc)
Attachment #542358 - Flags: review? → review?(roc)
There is also a "part 3" but I spawned that off as bug 667653,
since it orthogonal to this bug.
Duplicate of this bug: 666696
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.
Attachment #542358 - Flags: review?(roc) → review+
Comment on attachment 542359 [details] [diff] [review]
Part 2, tests

Review of attachment 542359 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #542359 - Flags: review?(roc) → review+
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/
(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?
(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.
Well, that's if you're using three dots instead of an ellipsis.
Created attachment 542846 [details] [diff] [review]
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.
Attachment #542358 - Attachment is obsolete: true
Attachment #542846 - Flags: review?(roc)
Created attachment 542848 [details] [diff] [review]
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.
Attachment #542359 - Attachment is obsolete: true
Attachment #542848 - Flags: review?(roc)
Comment on attachment 542846 [details] [diff] [review]
Part 1, clip selected text, background and selection decorations

Review of attachment 542846 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #542846 - Flags: review?(roc) → review+
Comment on attachment 542848 [details] [diff] [review]
Part 2, tests

Review of attachment 542848 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #542848 - Flags: review?(roc) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/e6f9648715c1
http://hg.mozilla.org/integration/mozilla-inbound/rev/80ecb84cc680
Flags: in-testsuite+
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/e6f9648715c1
http://hg.mozilla.org/mozilla-central/rev/80ecb84cc680
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 668849

Updated

6 years ago
tracking-firefox7: ? → ---
You need to log in before you can comment on or make changes to this bug.