Can't insert into blank table cell

VERIFIED FIXED in M17

Status

()

Core
Editor
P3
normal
VERIFIED FIXED
19 years ago
17 years ago

People

(Reporter: Akkana Peck, Assigned: Joe Francis)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

19 years ago
Save this html to a file and then load it into the editor:

<html>
<body>
<table>
<tr><th>one <th>two <th>three <th>four
<tr><td>first <td> <td>third
<tr><td>1 <td>2 <td>3 <td>4
</table>
</body>
</html>

Click in the empty space under "two" and over "2".  The caret blinks in the
right place.  Now type "a".  Bad things happened -- the whole middle row
disappears.

It's not really gone, though: OutputHTML gives me:

<body>
<table><tbody><tr><th>one </th><th>two </th><th>three </th><th>four
</th></tr><tr><td>first </td>a<td> </td><td>third
</td></tr><tr><td>1 </td><td>2 </td><td>3 </td><td>4
</td></tr></tbody></table>
</body>
</html>

So the "a" got inserted outside the <td> where it was supposed to be.  Not sure
what's up with layout that it's not showing the middle line in the editor (if I
call up the above html in the browser, I see all three rows of the table and the
"a" is before the beginning of the table) -- perhaps that should be a separate
bug.

Updated

19 years ago
Target Milestone: M13

Comment 1

19 years ago
setting to m13
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

19 years ago
Cool.  Somebody filed a bug I can use as a marker for a general issue:  There is
a lot of missing logic right now for making sure that operations are legal.
There is also some missing logic for attempting to resolve illegal operations by
tweaking the selection slightly.  These are features, or design issues, or
whatver you want to call them.  Let's just call them a lot of work.

But I want to do it soon; early m13.
(Assignee)

Comment 3

19 years ago
*** Bug 21404 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

19 years ago
Target Milestone: M13 → M14
(Assignee)

Comment 4

19 years ago
hmmm.  This bug is fixed, excpet for the part where it isn't. :-P

Seriously, though, this bug is fixed except for a very special case.  That case
actually occurs if you use the html shown at the start of this bug report.  If
that table cell had been _totally_ empty (not even a space in it), then
everything now works fine.

You see, I put special breaks into any empty table cell when we read them in.  To
tell if they are empty, I see if they contain any editable nodes.  A text ndoe
that is a single space is considered editable by nsEditor::IsEditable(), so I
think the cell isn't empty and doesnt need a special break.  But layout sees a
cell with just a space in it and decides that it is empty, and so doesn't even
create a frame for that cell.  Then selection can't find the cell when you click
on it, so the selection ends up in the table row.  the editor code sees you are
trying to type into a table row and wont let you (note that at least that much is
fixed - you no longer get text typed directly into the row - instead now nothing
happens).

Easy solution: make IsEditable() consider any text node that is all whitespace to
be not editable.  It's easy but it's wrong.  Layout sometimes considers
whitespace nodes to be significant and will render them.  We could just ask
layout if the text node has a frame associated with it.  That would work.  And in
fact we do that _if_ it's not a text node.  But its slow to ask layout that, and
so we tried to be clever and optimize the text node case since that gets called a
gizillion times.  If I make that change it will slow things down.

I don't know (yet) what to do about this, so I'm m14ing it.

Comment 5

19 years ago
The only suggestion I can think of is to replace a single space in a table cell
with an &nbsp; We know that forces a real cell that is displayed without cell
borders in Navigator. That should be done when parsing the input during page
loading, but that rule probably should also be enforced with any editing
change to a table cell (change in contents, new cell insertion, etc.)
(Reporter)

Comment 6

19 years ago
Please, let's not replace spaces with nbsp!

Could we ask layout whether the node has a frame only in the case where we
already think it's probably empty (whitespace only)?  If we could pre-check it
using our own fast heuristic, then maybe the perfomance issue won't be so
important, except in anomalous cases where someone tries to read in a huge table
where every node contains only whitespace, where we'd have to call layout on
every table cell.
(Assignee)

Comment 7

19 years ago
akkana, that sounds like a workable suggestion.  I'll give that a go.
(Assignee)

Comment 8

18 years ago
hmmm.  I tried changing IsEditable(), and got very unexpected results.  Even 
though layout does not render the space in the (otherwise empty) table cell, if I 
ask it to get the primary frame for that text node it returns one.  I dont know 
what frame this is (perhaps the frame for the cell?).  

I'm going to have to M15 this and talk to the layout folks.
Target Milestone: M14 → M15

Comment 9

18 years ago
moving to M17
Target Milestone: M15 → M17
(Assignee)

Comment 10

18 years ago
fixed; thanks to mjudge for doing the real work here...
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Updated

18 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 11

18 years ago
We had to back out the fix for this since it was causing data loss for
operations like bold.

The frame data the fix depends on is not valid sometimes because it needs to be
reflowed. In the case of the bold data loss, CheckVisibility() was returning
PR_FALSE for visible because the frame it was looking at had a zero offset and
zero length.

Reopening bug.

(Assignee)

Comment 12

18 years ago
fixed.  see bug 46209 for a related issue that is still open.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Comment 13

18 years ago
verified in 9/29 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.