Last Comment Bug 666669 - Text under the text-overflow marker (ellipsis) is displayed (overlaps) when text is selected
: Text under the text-overflow marker (ellipsis) is displayed (overlaps) when t...
Status: RESOLVED FIXED
[inbound]
: css3
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: -- minor with 4 votes (vote)
: mozilla7
Assigned To: Mats Palmgren (vacation)
:
Mentors:
: 666696 (view as bug list)
Depends on: 668849
Blocks: 312156 666696 667653
  Show dependency treegraph
 
Reported: 2011-06-23 11:19 PDT by Mats Palmgren (vacation)
Modified: 2011-07-05 18:54 PDT (History)
16 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial Patch (3.41 KB, patch)
2011-06-24 16:08 PDT, Tom Lee
mats: review-
Details | Diff | Splinter Review
Part 1, clip selected text, background and selection decorations (13.76 KB, patch)
2011-06-27 19:12 PDT, Mats Palmgren (vacation)
roc: review+
Details | Diff | Splinter Review
Part 2, tests (8.45 KB, patch)
2011-06-27 19:13 PDT, Mats Palmgren (vacation)
roc: review+
Details | Diff | Splinter Review
Part 1, clip selected text, background and selection decorations (15.83 KB, patch)
2011-06-29 09:42 PDT, Mats Palmgren (vacation)
roc: review+
Details | Diff | Splinter Review
Part 2, tests (10.00 KB, patch)
2011-06-29 09:47 PDT, Mats Palmgren (vacation)
roc: review+
Details | Diff | Splinter Review

Description Mats Palmgren (vacation) 2011-06-23 11:19:34 PDT
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+2 (mostly busy through August 4; review requests must explain patch) 2011-06-23 16:51:05 PDT
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] 2011-06-24 02:26:45 PDT
(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) (Exited; email my personal email if necessary) 2011-06-24 02:29:50 PDT
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 2011-06-24 16:08:09 PDT
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!
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-25 03:50:07 PDT
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) 2011-06-25 17:59:12 PDT
(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 (vacation) 2011-06-27 19:06:54 PDT
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 (vacation) 2011-06-27 19:12:46 PDT
Created attachment 542358 [details] [diff] [review]
Part 1, clip selected text, background and selection decorations
Comment 9 Mats Palmgren (vacation) 2011-06-27 19:13:37 PDT
Created attachment 542359 [details] [diff] [review]
Part 2, tests
Comment 10 Mats Palmgren (vacation) 2011-06-27 19:22:11 PDT
There is also a "part 3" but I spawned that off as bug 667653,
since it orthogonal to this bug.
Comment 11 Mats Palmgren (vacation) 2011-06-27 19:24:18 PDT
*** Bug 666696 has been marked as a duplicate of this bug. ***
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-27 20:13:39 PDT
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) (Exited; email my personal email if necessary) 2011-06-27 20:14:39 PDT
Comment on attachment 542359 [details] [diff] [review]
Part 2, tests

Review of attachment 542359 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 14 Mats Palmgren (vacation) 2011-06-27 20:16:04 PDT
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/
Comment 15 Mats Palmgren (vacation) 2011-06-27 20:24:35 PDT
(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) (Exited; email my personal email if necessary) 2011-06-27 21:02:27 PDT
(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) (Exited; email my personal email if necessary) 2011-06-27 21:02:47 PDT
Well, that's if you're using three dots instead of an ellipsis.
Comment 18 Mats Palmgren (vacation) 2011-06-29 09:42:10 PDT
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.
Comment 19 Mats Palmgren (vacation) 2011-06-29 09:47:00 PDT
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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-29 17:21:59 PDT
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) (Exited; email my personal email if necessary) 2011-06-29 17:27:15 PDT
Comment on attachment 542848 [details] [diff] [review]
Part 2, tests

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

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