333481 - Selecting text inside table cell with the mouse makes the selection expand over the following rows

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Frank Wein [:mcsmurf]]

To reproduce:
1. Open the testcase
2. Try to select the text "Bug 29856. Support SetWindowClass in GTK2. r+sr=roc, patch-midden by Andrew Taylor, Arik Devens, Rob Ginda, Kenneth Herron, timeless, Alex Zbyslaw, and me." within the table cell with the mouse

Results:
As soon as you move your mouse cursor below or behind the text (while still holding the left mouse button to mark text and staying within the table cell with the mouse), the text in the table cells in the other columns below the mouse cursor also gets marked. Normally only the text inside this table cell should be marked as long as the (mouse) cursor stays inside the table cell.

This looks like a regression from Bug 317375 (tested via narrowing down a regression range).

Comment 1

[Frank Wein [:mcsmurf]]

Attachment: Testcase

Comment 2

[Bernd]

Robert, thats basically why we had the code mentioned in bug 219147 in our tree. Shall I vandalize your code or will you do it yourself?

Comment 3

[Eli Friedman]

Attachment: Testcase that doesn't use selection

Better testcase so that the issues don't get confused.  There's a selection issue for which I never filed a bug that table rows don't account for rowspanned kids; try turning caret browsing on and clicking to the right of the table to see what I mean.  That issue is basically independent of the issue here, which is that the frame targeting code isn't targeting the correct frame.

Comment 4

[Bernd]

from the old code....

/* we overload this here because rows have children that can span outside of themselves.
 * so the default "get the child rect, see if it contains the event point" action isn't
 * sufficient.  We have to ask the row if it has a child that contains the point.
 */


and this should be implemented at http://lxr.mozilla.org/seamonkey/source/layout/tables/nsTableRowFrame.cpp#596

 to fix this, I guess.

Comment 5

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

Actually, no.

Setting gDumpEventList to 1 in a debugger is a great way to debug problems like this. Here's what we get when the cursor is in the troublesome part of the frame:

Background 0xd9dbc0(HTMLScroll(html)(-1)) (0,0,13275,11805) uniform
Clip (nil)() (0,0,13275,11805)
    CanvasBackground 0xd9d970(Canvas(html)(-1)) (0,0,13275,11805) uniform
Clip (nil)() (0,0,13275,11805)
    Background 0x2aaab1ecebe8(Area(html)(-1)) (0,0,13275,1050) uniform
    Background 0x2aaab1eceea8(Block(body)(1)) (120,120,13035,810) uniform
    TableBorderBackground 0xe03418(Table(table)(0)) (120,120,6090,810)
    Background 0xe03418(Table(table)(0)) (120,120,6090,810) uniform
    Background 0xe03590(TableRowGroup(tbody)(1)) (165,165,6000,720) uniform
    Background 0xe03a50(TableRow(tr)(0)) (165,165,6000,345) uniform
    TableCellBackground 0xb61468(TableCell(td)(2)) (3300,165,2865,720)
    Border 0xb61468(TableCell(td)(2)) (3300,165,2865,720)
    Background 0xb61590(Block(td)(2)) (3330,240,2805,570) uniform
    Background 0xb619e8(TableRow(tr)(1)) (165,540,6000,345) uniform

This shows that we are finding the table cell, but unfortunately we're also finding the second row, and treating it as on top of the cell. We need to fix the z-ordering, because table cell backgrounds should be above all table row backgrounds (see http://www.w3.org/TR/CSS21/zindex.html).

Comment 6

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

Attachment: fix

This is a clean fix.

Comment 7

[Bernd]

Whoaa , blackmailing with my coding abilities works.

I wonder how 
http://lxr.mozilla.org/seamonkey/source/layout/base/nsLayoutUtils.cpp#612
and 
http://lxr.mozilla.org/seamonkey/source/extensions/layout-debug/src/nsILayoutDebuggingTools.idl#64
relate, shouldn't some of the values in the first one be used instead of the old ones?

Comment 8

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

Sure, feel free to hook them up!

Comment 9

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

checked in.

Comment 10

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

Should the stuff that the table background painter does be represented in terms of display lists to avoid needing to do this?

Comment 11

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

We probably could. However, I think we have enough cans of worms open at the moment that I'm not going to do it. If we did, I think it would mean having the table painter adopt the approach here.

Comment 12

[Peter van der Woude [:Peter6]]

did this cause bug 335449 ?

Comment 13

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

...or bug 334690?