306895 - Triple click should select lines, not paragraphs, in "white-space: -moz-pre-wrap;"

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Bug 32807 made triple-click select paragraphs, not lines.  This is a good idea
in general, but for preformatted text it's not so great.  Also, it makes
textareas much harder to use, I think... I think bug 32807 comment 43 sums up
the situation well; the relevant part is:

  Triple click selects the logical group under the pointer. For something like a
  text editor, or a terminal emulator, that's the current line. HTML, however
  has no concept of lines. Text flows, and wraps.

Note that a textarea _is_ a text editor and that preformatted text does not in
fact flow and wrap.

I have no opinion on what should happen for -moz-pre-wrap text in general...

Comment 1

[Uri Bernstein]

For preformatted text (such as the comments on this bug report), I already made
triple-click select lines, not paragraphs, as part of the patch that got checked
in for bug 32807. So this WFM - and I'm not sure what the request here is for.

As for textareas - text in fact does flow and wrap in them. So I thought
allowing paragraph selection in them is useful. I still think so. This is also
what Safari does (it would be nice if someone can check other browsers, such as IE).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm... I was having a problem with preformatted text, but now I can't reproduce
it...  That said, I did do some testing and while we do the right thing for
"white-space: pre" we don't do the right thing for "white-space: -moz-pre-wrap".
 The URL field has the testcase for that.  Maybe I was experimenting with the
sort of markup we'd get in a BR-free text editor when I ran into issues.  Do you
want to handle that in this bug, or should I file a new bug?  Testcase is:

data:text/html,<div style="white-space: -moz-pre-wrap;">Triple click
this&#xA;This should not be selected

As for IE6/Win, it does seem to have the same behavior in textareas as we
currently do.  It's pretty annoying (I've been doing review comments and such
with this behavior, and it's a pain), but I guess that's just one more reason
for me to stop using textareas altogether...

Comment 3

[Boris Zbarsky [:bzbarsky]]

For the -moz-pre-wrap thing, I suspect you want to replace:

1594   if (selectPara && 
1595       NS_STYLE_WHITESPACE_PRE == mStyleContext->GetStyleText()->mWhiteSpace)
1596     selectPara = PR_FALSE;

with

1594   if (selectPara && mStyleContext->GetStyleText()->WhiteSpaceIsSignificant())
1596     selectPara = PR_FALSE;

Comment 4

[Uri Bernstein]

(In reply to comment #2)
>The URL field has the testcase for that.

Now it does.

(In reply to comment #3)
> For the -moz-pre-wrap thing, I suspect you want to replace [...]
 
But that would also trigger your preferred behavior in textareas, since they use
-moz-pre-wrap. That's the reason I purposely avoided doing so in the first place
(I consider paragraph-select in textareas to be correct and useful. And it's not
just IE/Win, Safari also behaves this way. Opera/Mac selects the entire textarea
on triple-click).

On the other hand, fixing the -moz-pre-wrap bug without breaking ("fixing")
textareas is difficult. But then again, it's a pretty minor bug IMO, since
-moz-pre-wrap is rarely used other than in textareas, as far as I know.

I'll try to work on implementing the "correct" (and difficult) solution. But
assuming for now I can't do it, what should I do in the meantime? (The options
are leaving this as it is, or implementing your suggestion).

Comment 5

[Uri Bernstein]

Attachment: patch v1.0

This is the "correct" solution: for pre-formatted text frames, look for a
newline character at the end of the frame, and if one is found, stop the
selection there.

This fixes the -moz-pre-wrap case, as well as the case of an inline
preformatted element containing a line break.

This does NOT change triple-click behavior in textareas (altough it does ensure
that the current behavior will persist even if we get rid of the BRFrames
currently inserted into textares).
Boris, if you feel strongly about changing the behavior in textareas, I suggest
that you file a separate bug.

Question:
Having a "nsCOMPtr<nsIContent>*" argument is ugly. Is there an elegant way to
avoid it?

Comment 6

[Boris Zbarsky [:bzbarsky]]

You could return the nsIContent and have the PRBool be the out param....  That
seems like it would be the simplest way.  Or you could make the return value be
a struct that contains the nsIContent*, PRBool, etc.

Is there a reason to add methods to nsIFrame instead of looking at the frame
type and casting to nsTextFrame?  Just the fact that nsTextFrame is not actually
declared in a header?

Comment 7

[Uri Bernstein]

(In reply to comment #6)
> You could return the nsIContent and have the PRBool be the out param....  That
> seems like it would be the simplest way.  Or you could make the return value be
> a struct that contains the nsIContent*, PRBool, etc.
> 

Hmmm... I don't really like either of these solutions (the nsIContent and the
contentOffset are closely related, so I don't want to separate them, and I don't
want to complicate things by defining another struct). So unless you strongly
prefer one of those over the current patch, I'll just leave it like this.

> Is there a reason to add methods to nsIFrame instead of looking at the frame
> type and casting to nsTextFrame?  Just the fact that nsTextFrame is not actually
> declared in a header?

Yes, the fact that it's not declared in a header is certainly a problem (how
would I overcome that?) But also, I thought casting was ugly, and I also noticed
that GetOffsets() was done this way (defined for nsIFrame although it's only
meaningful for nsTextFrame) - so I followed that example. 
If you have a solution to the no-header problem, and you prefer that I use
casting, let me know and I'll do it.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> how would I overcome that?

Move the class decl to nsTextFrame.h.  It's well past time we did that.

> But also, I thought casting was ugly

While perhaps true, I think it's better than adding methods to nsIFrame that
only make sense for one single concrete frame class...  Which if nothing else
bloats every single frame vtable.

Comment 9

[Uri Bernstein]

Attachment: patch v1.1

Move the nsTextFrame class declaration to nsTextFrame.h, so I can just cast to
nsTextFrame* (after checking the Type) and leave nsIFrame alone.

Comment 10

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

I strongly prefer a struct for the result...

other than that it looks fine.

Comment 11

[Uri Bernstein]

Attachment: patch v1.2

Use a struct for return values from FindBlockFrameOrBR().
I hope this is what you had in mind. I personally don't care much for it.

Comment 12

[Uri Bernstein]

Removing "and textareas" from summary. Let's leave textareas out of this bug.

Comment 13

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

Comment on attachment 196423 [details] [diff] [review]
patch v1.2

Instead of an mFound field, you can just use mContent == nsnull for "not
found".

It's not the greatest --- too bad C++ doesn't support real tuples --- but the
content+offset pair is a reasonable thing to treat as one unit of data, IMHO.

Comment 14

[Uri Bernstein]

Attachment: patch v1.3

Yes, getting rid of mFound certainly makes it neater.
I avoided "== nsnull" comaprisons, as I was once told to do (bug 277553 comment
#26). I hope that's OK.

Asking for the reviews again, to be on the safe side.

Comment 15

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

Comment on attachment 196507 [details] [diff] [review]
patch v1.3

great!

Comment 16

[Uri Bernstein]

Attachment: patch v1.3, unbitrotten

Same as v1.3, brought up to date with latest changes to trunk.
Carrying over review flags, since nothing changed in the patch itself.

Comment 17

[Uri Bernstein]

Checked in by Olli.Pettay%helsinki.fi (smaug) at 2005-09-18 05:41.