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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: mcsmurf, Assigned: roc)

Tracking

({regression, testcase})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

13 years ago
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).
Reporter

Comment 1

13 years ago
Posted file Testcase (obsolete) —

Comment 2

13 years ago
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

13 years ago
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.
Attachment #217909 - Attachment is obsolete: true

Comment 4

13 years ago
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.
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).
Posted patch fixSplinter Review
This is a clean fix.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #218105 - Flags: review?(bernd_mozilla)

Comment 7

13 years ago
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?
Sure, feel free to hook them up!

Updated

13 years ago
Attachment #218105 - Flags: review?(bernd_mozilla) → review+
checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Should the stuff that the table background painter does be represented in terms of display lists to avoid needing to do this?
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.
Depends on: 334942
Depends on: 334945
Depends on: 334690
Depends on: 335030
did this cause bug 335449 ?
Depends on: 335449
You need to log in before you can comment on or make changes to this bug.