58704 - cannot select anything smaller than lines in justified text

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect, P1)

Description

[Chris Siebenmann]

Build ID: a trunk CVS pull from today

 Current Mozilla builds don't seem to be able to select in justified text
properly (or at least justified text in tables, which is what I have boiled
down a simplified test case to). It appears to work like this: in the first
line you can select normally. After that, you can only select line-by-line,
and if you double-click to select a word the first word of the line will be
selected, no matter where the cursor is.

 I will attach a test case. An in-the-field case is at any site that uses
justified text content, such as the stories on http://www.theregister.co.uk/

 This is a recent regression. I believe it appeared after the fix for a
corruption bug involving selection of justified text was put into the trunk,
within the past week or so. (Bug #56704)

Comment 1

[Chris Siebenmann]

Attachment: Justified text test case for selection

Comment 2

[Cyd]

Confirming on Linux build 2000103108.

Comment 3

[Matthew T (active 1999-2002)]

*** Bug 60363 has been marked as a duplicate of this bug. ***

Comment 4

[Matthew T (active 1999-2002)]

[Reproduced on build 2000112320, Mac OS 9.0 --> All/All.]

Comment 5

[rubydoo123]

using the build from 12/31 on win98 I am unable to reproduce this bug. I can 
select a word or line based on clicks and can select any word in any line. 
marking wfm

Comment 6

[Chris Siebenmann]

 This bug remains present in a current Linux CVS build. Reopening.
The instructions in the attachment remain current for me.

Comment 7

[rubydoo123]

reassign to akkana to check this out on linux, akkana if you can dup this one, 
can you please update the bug and then hand it over to anthony -- he may need 
some guidance as to the root of the issue since it seems to be linuz specific

Comment 8

[Akkana Peck]

Yes, it happens on linux exactly as described.  Weird!  Matthew said he saw it
on Mac as well, so it doesn't sound like it's linux only.

Comment 9

[rubydoo123]

this is still an issue on mac using the build 2001011912 -- really weird 
selection bug

Comment 10

[anthonyd]

setting to mozilla 0.9.1

Comment 11

[Kathleen :Brade]

move to mozilla0.9.2
I wonder if there is some pref setting that is different on Windows (from Mac and 
Unix) and that is what causing the platform difference?

Comment 12

[rubydoo123]

moving to 1.0

Comment 13

[Alex Stewart]

*** Bug 89579 has been marked as a duplicate of this bug. ***

Comment 14

[Alex Stewart]

*** Bug 62490 has been marked as a duplicate of this bug. ***

Comment 15

[Alex Stewart]

An interesting note: if the justified block contains an inline element, then you
can begin or end a selection anywhere inside the inline element, as normal.

Comment 16

[nyh]

Yes, and another example weirdness: if the (second or later) justified line
contains a link (A HREF), double-clicking somewhere in the last after the </A>
chooses the last word in the link text (not the word under the mouse, and not
the first word of the line). Moreover it seems the word after the link sometimes
gets underlined! Very Weird.

BTW, the bug is still present in 0.9.3, Linux (redhat 7.1, RPM)

Comment 17

[Ilya Tsindlekht]

Attachment: Test case with italics text inside justified paragraph

Comment 18

[Ilya Tsindlekht]

Also, if there is an italics text inside the justified paragraph, that 
text can be selected normally. I have added a testcase showing it.

Comment 19

[Boris Zbarsky [:bzbarsky]]

*** Bug 53334 has been marked as a duplicate of this bug. ***

Comment 20

[Boris Zbarsky [:bzbarsky]]

*** Bug 95757 has been marked as a duplicate of this bug. ***

Comment 21

[rubydoo123]

--> mjudge

Comment 22

[Boris Zbarsky [:bzbarsky]]

*** Bug 99516 has been marked as a duplicate of this bug. ***

Comment 23

[Boris Zbarsky [:bzbarsky]]

*** Bug 112067 has been marked as a duplicate of this bug. ***

Comment 24

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 25

[mjudge]

*** Bug 96281 has been marked as a duplicate of this bug. ***

Comment 26

[Boris Zbarsky [:bzbarsky]]

*** Bug 114600 has been marked as a duplicate of this bug. ***

Comment 27

[Michael Karcher]

An interesting thing is: If I (Mozilla 0.9.7 on Debian Linux) try to select into
the last line of the second paragraph in the first example, it is possible to
select up to any letter by moving the mouse downward. If the mouse cursor is in
the pixel row below the last line, you can select just as it should be.

Comment 28

[Boris Zbarsky [:bzbarsky]]

*** Bug 124473 has been marked as a duplicate of this bug. ***

Comment 29

[Blake Ross]

changing selection qa to tpreston.

Comment 30

[Boris Zbarsky [:bzbarsky]]

*** Bug 130517 has been marked as a duplicate of this bug. ***

Comment 31

[Boris Zbarsky [:bzbarsky]]

*** Bug 137488 has been marked as a duplicate of this bug. ***

Comment 32

[Boris Zbarsky [:bzbarsky]]

Nominating.  This is the sort of behavior that end-users find incredibly
frustrating.  User points at a word, clicks, drags right, nothing happens.  User
points at a word, clicks, drags down, the entire line (before and after the
point where user clicked) is selected.  User concludes that the program has
screwy selection behavior and never selects text in it again, since it seems to
be impossible to do it in a reasonable way.

As a note, this needs to be tested on a line that is _not_ the first line in the
paragraph (leading to yet more confusion).

Could someone point me to the code that would be involved in this, please? 
mjudge?  jfrancis?  akkana?

Comment 33

[Akkana Peck]

I second the nomination  -- this happened to me the other day and was indeed
extremely frustrating (one of those "What kind of brain-dead app is this?" moments).

Most text selection is done in nsTextFrame.cpp and nsFrame.cpp (or was, last
time I tried to look at selection bugs).  It was never clear to me what
controlled what was in one file vs. the other -- for example, word delimiters
were handled in nsTextFrame but paragraph delimiters were in nsFrame.  

Comment 34

[saari (gone)]

nominating for EDITORBASE

Comment 35

[Kevin McCluskey (gone)]

editorbase triage: EDITORBASE-. The default for tables is align=left. This is
not common usage.

Comment 36

[Kathleen :Brade]

Please re-evaluate.  There are 12 duplicate bugs for this bug; are we really
sure it's not common editor usage?

Comment 37

[Håkan Waara]

*** Bug 138261 has been marked as a duplicate of this bug. ***

Comment 38

[Boris Zbarsky [:bzbarsky]]

As a note, this is the default for all blog entries and a number of news
sites...  News sites in particular tend to go for a "newspaper-like" look, which
means align:justify.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch v1.0 (special thanks to kin, hwaara and especially smontagu)

OK... This is a bug on Mac and Linux, but not Windows because on Windows the
"browser.drag_out_of_frame_style" pref is not set to "1".  This pref is checked
by nsTextFrame::GetPositionSlowly(), and if it's true we perform a check to see
whether the position is inside the frame bounds at all.  Now the checking code
looks like this:

nsRect bounds(mRect);
bounds.MoveBy(origin.x, origin.y);
if (aPoint.y < bounds.y)//above rectangle
{
  aOffset = mContentOffset;
  outofstylehandled = PR_TRUE;
}
else if (aPoint.y > (bounds.y + bounds.height))
{
  aOffset = mContentOffset + mContentLength;
  outofstylehandled = PR_TRUE;
}

After this, if |outofstylehandled| is true we just use the aOffset value we
already have instead of calculating it.

Now testing on the testcases here, clicking on the _first_ line I get:
  origin.y == 930
  bounds.y (before adjustment) == 0
  bounds.height == 238

On the _second_ line I get:
  origin.y == 1168
  bounds.y (before adjustment) == 238
  bounds.height == 238

So the "238" height has been counted twice -- "origin" has moved and the origin
of mRect has moved (naturally).  As a result, we hit the |aPoint.y < bounds.y|
case, set the offset to the smallest offset in the frame, and things break.

The attached patch changes this code to be just like the code in
nsTextFrame::GetPosition().  It fixes the bug for me and seems like the right
thing to do from a completely theoretical standpoint...

The only caveat is that the current (buggy, imo) code was put in place in
revision 1.270 of nsTextFrame.cpp to fix Mac bugs: bug 29570, bug 37393, bug
42794.	It replaced other (buggy, obviously) code that used mRect.y where I use
origin.y in the patch.	I'm 99% certain that using origin.y is the right thing
to do, but someone with a Mac should test the three bugs in question with the
patch.	brade?	Can you do it?

Comment 40

[Håkan Waara]

I played around with the patch for a while, and it seems to work well. 

(One problem I noticed however, was that dragging text and dropping it at the
drag-location crashed Mozilla. However, this seems to happen even *without* the
patch applied...)

Comment 41

[R.K.Aa.]

*** Bug 139446 has been marked as a duplicate of this bug. ***

Comment 42

[Brendan Eich [:brendan]]

mjudge, can you review bz's patch, or name another reviewer?  We should get this
in for 1.0RC2.  Thanks,

/be

Comment 43

[Kathleen :Brade]

Hakan--by some chance were you using Linux when you encountered the crasher you
mention in comment 40?  There is a known issue (Linux-specific) with drag/drop.
 I believe the bug now has a reviewed patch and may be checked in now (or maybe
not?).  I'm pretty sure it's a topcrash.  Sorry I don't have the bug # handy.

Comment 44

[Håkan Waara]

Yes, I was using Linux. (Can we please get some review action in this bug? It's
pretty major for all Linux users, I'd say.)

Comment 45

[Adrian Ulrich]

Hakan:

Thats bug 81779

Comment 46

[Kathleen :Brade]

bug 81779
I just got e-mail indicating it was just checked in

Comment 47

[Håkan Waara]

This is wanted by drivers for RC2, I tested the patch (it works fine) and we're
still not getting any review attention.  I'm tempted to just stamp my r=, since
I tested it, but I won't.  mjudge, can you please have a look at the patch?

Comment 48

[kinmoz]

Comment on attachment 80111 [details] [diff] [review]
Proposed patch v1.0 (special thanks to kin, hwaara and especially smontagu)

I'm going to go ahead and sr=[e-mail] this patch. I think that in the
original patch, 
mjudge meant to use MoveTo() instead of MoveBy(), which would have made the 
coordinates involved equivalent to the code in GetPosition() ... only it used 2 
extra function calls to get the bounds and reset the origin of the resulting 
bounds rect.

I don't think we have to worry about regressing any of the bugs the original
patch was intended to fix. aPoint is in the coordinate system of the view not
the frame, and the rect we are comparing this point with is also in the view
coordinate system (with bz's patch) so the intended comparison's should work
properly.

Comment 49

[Akkana Peck]

Comment on attachment 80111 [details] [diff] [review]
Proposed patch v1.0 (special thanks to kin, hwaara and especially smontagu)

We don't seem to be hearing from Mike (though he said on Wed. and Thurs. that
he would look at it) so I'll give it my r=akkana.  Everything I've tested with
the patch works as expected, no unwanted side effects noticed.

Comment 50

[mjudge]

got laptop back and i have been wrestling with the bulid system on it. If akkana 
has reviewed it then thats cool with me.

Comment 51

[Asa Dotzler [:asa]]

Is this ready to go? Can it be landed today or tomorrow?

Comment 52

[Boris Zbarsky [:bzbarsky]]

To me.  Checked in on trunk, mailed drivers for branch approval.

Comment 53

[Asa Dotzler [:asa]]

Comment on attachment 80111 [details] [diff] [review]
Proposed patch v1.0 (special thanks to kin, hwaara and especially smontagu)

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 54

[Boris Zbarsky [:bzbarsky]]

checked in on the branch.

Comment 55

[Nick Kocharhook]

This is great! Finally, I'm able to properly select justified text!

Thanks Boris! :-)

Comment 56

[saari (gone)]

is this checked in on trunk as well?

Comment 57

[Boris Zbarsky [:bzbarsky]]

Yep.  It is.  See comment 52. 

Comment 58

[Jacob Kjome]

hmmm... when I double click on words in the testcases, what gets highlighted is
the beginning of the word to the end of the word + 1 space.  In normally aligned
text, I just get from the beginning to the end of the word with no extra space
highlighted.

Is this is a bug in the patch that was commited?

Jake

Comment 59

[Boris Zbarsky [:bzbarsky]]

hoju, what OS is this?  I cannot reproduce that on Linux....

In any case, it's a separate bug from this one. 

Comment 60

[Akkana Peck]

The space is selected deliberately on windows.  Linux (and, I think, mac) don't
select the space.  It's controlled by the prefs:
layout.word_select.eat_space_to_next_word and
layout.word_select.stop_at_punctuation.

Comment 61

[Jacob Kjome]

Akkana,

Why the difference in behavior for different platforms?  Why the difference in 
selection between text with different alignments?  And why would one want a 
preference like this in the first place?  Shouldn't it just behave the same?  
BTW, Boris, I am on Win2k (sp2sr1).

Jake

Comment 62

[Akkana Peck]

People have different expectations on different platforms, because the native
apps have different behaviors.  We try to mimic native app selection behavior as
much as possible.

For some of the history of the doubleclick behavior, see bug 98546.  Note that
selecting the space was what all platforms did at first, because that was what
Windows users expected and the code was written by a windows person, but
eventually a mac person fixed it to allow for the unix/mac expectation.  If you
disagree about the windows default, bug 98546 is the bug you want; but be ready
to back it up with a substantial list of popular windows apps that don't select
the space.

Comment 63

[Håkan Waara]

Tried selecting text on a few sites that used to suffer from this problem, they
all work flawlessly.  Yay!