Closed Bug 22227 Opened 25 years ago Closed 24 years ago

Can't insert into blank table cell

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: akkzilla, Assigned: mozeditor)

References

Details

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.
Target Milestone: M13
setting to m13
Status: NEW → ASSIGNED
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.
*** Bug 21404 has been marked as a duplicate of this bug. ***
Target Milestone: M13 → M14
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.
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.)
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.
akkana, that sounds like a workable suggestion.  I'll give that a go.

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
moving to M17
Target Milestone: M15 → M17
fixed; thanks to mjudge for doing the real work here...
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.


fixed.  see bug 46209 for a related issue that is still open.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
verified in 9/29 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.