208900 - Enter key is a NO-OP in some docs, since Mozilla 1.2

Status: RESOLVED INCOMPLETE

(Core :: DOM: Editor, defect)

Description

[Kai Engert [:KaiE:]]

Mozilla 1.0 works correctly.
I can reproduce this bug with 1.2, 1.3, 1.4 and latest trunk.

Go to http://www.mozilla.org/start/
Press CTRL-E to edit.
Place the caret somewhere into the middle of the paragraph below the "Report
Bugs" heading.
Hit enter.

Actual behaviour:
  Nothing happens

Expected behaviour:
  A newline should get inserted and split the paragraph at the
  caret position.

Comment 1

[Kai Engert [:KaiE:]]

This bug is also seen on the special 1.2 embedding branch.

Comment 2

[Kai Engert [:KaiE:]]

Attachment: testcase

Minimal test case, also thanks to Neil for spending some time on this.

Open the attached test case in editor, place the caret between a and b, hit
enter, nothing happens.

Actually, enter doesn't seem to have an effect anywhere in this document.

Comment 3

[Kai Engert [:KaiE:]]

Daniel and Neil discussed on IRC, and think this is valid HTML.

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Absolutely... 
HTML 4.01 DTD says:

   <!ELEMENT DD - O (%flow;)* -- definition description -->

where

   <!ENTITY % flow "%block; | %inline;">
   <!ENTITY % block
           "P | %heading; | %list; | %preformatted; | DL | DIV | NOSCRIPT |
           BLOCKQUOTE | FORM | HR | TABLE | FIELDSET | ADDRESS">

Comment 5

[kinmoz]

The problem is that the parser and the editor disagree on the "Blockness" of
<dd>. I modified the disagreement check in nsHTMLEditor::NodeIsBlockStatic() to
also reset the |*aIsBlock| value to what the editor thinks it should be, and
things work ok.

Comment 6

[kinmoz]

I should've mentioned above that the parser is saying dd is not a block, and the
editor thinks it should be. You should see lots of these assertions in the
console when using the testcase in composer.

###!!! ASSERTION: Parser and editor disagree on blockness: dd: '*aIsBlock', file
 y:/mozilla/editor/libeditor/html/nsHTMLEditor.cpp, line 694
Break: at file y:/mozilla/editor/libeditor/html/nsHTMLEditor.cpp, line 694

Comment 7

[kinmoz]

Kai and I were talking on IRC about this bug, and he asked me to update the bug
with some of the comments I was making ...

Use of the parser for blockness checks was turned on by the patch in bug 132352.

I'd probably look into why the parser is saying dd isn't a block, and while I
was at it, also check what other blocks (mentioned in the list used by the
editor to trigger the assertion) have the same problem.

If for some reason we can't fix/modify the parser, we can always compensate by
adjusting the *aIsBlock value to what we think it should be before returning
from NodeIsBlockStatic.

Comment 8

[Samir Gehani]

adt: nsbeta1+/adt3

Comment 9

[Kevin McCluskey (gone)]

editorbase+ for investigation of where we have a mismatch between the editor and
parser regarding block elements.

Comment 10

[Kai Engert [:KaiE:]]

Short story: I suspect the html parser's element table should include kDLChild
as  a parent for eHTMLTag_dd.


Long story:
Trying to find out why the parser things that <dd> is not a block element.

The html parser queries an element table to find out whether an item is a block.
In the element table, items are assigned to groups, named parents. As of now,
the DD element is classified as being a kInlineEntity.

Looking at CVS blame,
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/htmlparser/src/nsElementTable.cpp
the last change to the parent categorization was done in revision 3.134 with bug
106746.

The patch attached to bug 106746 did NOT! change the parent categorization.
However, the actual patch checked in by Harish DID change it!
Harish, do you think that happened by mistake?
I see that your patch removed "kDLChild" from "parent" and "incl", and it also
removes the comment mentioning but 102370, but I do not see any explanation in
bug 106746 or its patches that this change was planned.

If kDLChild was removed by mistake, a first test would be to add it back.
I will try that out now.
However, I don't have a lot of hope, because I see the parser service's IsBlock
implementation does not check for the kDLChild flag, so what I mention here
might be completely unrelated and adding it back might not fix the bug.

Comment 11

[Kai Engert [:KaiE:]]

Reassigning to Harish who offered to investigate.

We suspect I'm wrong, but I have the feeling that nsParserService::IsBlock might
be broken, and should not call IsMemberOf but ContainsSet...?

Doing some experimentation, I was able to "fix" the bug by doing this change to
IsBlock, and also changed the Initialization call in nsElementsTable.cpp to use
kFlowEntity for the parent parameter (instead of kInlineEntity).

But again, I can't really say I understand the code. :-)

Comment 12

[harishd]

>We suspect I'm wrong, but I have the feeling that nsParserService::IsBlock might
>be broken, and should not call IsMemberOf but ContainsSet...?

I don't think IsBlock calling IsMemberOf is incorrect however I do believe that
just calling IsMemeberOf isn't sufficient. That's, to determine if an element is
block level we should do the following:
1) Check whether the element is a block level element by calling IsMemberOf. 
2) If IsMemberOf cannot determine then check whether the current element can   
  contain block level elements. 

Comment 13

[harishd]

Attachment: Patch v1.0

Consider an element a block-element if it can contain block level elements.

Comment 14

[Kai Engert [:KaiE:]]

Comment on attachment 127015 [details] [diff] [review]
Patch v1.0

Harish's reasoning makes sense to me. I have tested the patch and it fixes the
bug.

Harish said it is unlikely this introduces regressions.

r=kaie

Comment 15

[kinmoz]

Comment on attachment 127015 [details] [diff] [review]
Patch v1.0

So I did some poking around in nsElementTable.cpp because I was curious about
what tags this change would affect. Here's the list I came up with:

  map
  applet
  blink
  body
  button
  caption
  counter
  dd
  dt
  iframe
  nobr
  object
  parsererror
  plaintext
  sourcetext
  span
  td
  th

The one that I'm concerned with the most is "span". kaie, have you done any
tests to see if spans are treated as blocks with this change?

Comment 16

[Kai Engert [:KaiE:]]

Kathy and Kin discussed on IRC about this bug and what should get tested, and
Kathy tested. Thanks a lot to both.
I think the conclusion is the patch works fine.
Here is an excerpt from the discussion that shows which cases where looked at:

<kin> kaie: startup composer in CSS mode (which is the default) it uses spans
for everything
<kin> kaie: and try and insert blocks while in spans etc
<kin> basically I'm wondering if spans are treated as blocks now
<kin> they are inline elements
<kaie> what would be the effect if they are treated as blocks?
<kin> but if you look at the element table in the parser,they do light the
kBlock bit that harish's change checks
<kin> kaie: splitting of blocks won't propogate up to the proper block parent
<kin> kaie: basically there are some operations where the editor searches up
from where the caret/selection is
<kin> to find the "block parent"
<kin> if you are in a span, I'm wondering if it will now stop at the span
<kin> because of harish's changes
<kaie> Do you mean <h1><span style="">a|b</span></h1> when pressing enter
between a und b, the h1 might not get duplicated?
<kaie> kin: do you think of something like that?
<kin> kaie: maybe something like
<kin> <ul><li><span>a|b</span></li></ul>
<kin> though I would verify that hitting return in <ul><li><div>a|b</div></li></ul>
<kin> inserts does this first:
<kin> <ul><li><div>a<br>b</div></li></ul>
<brade> kin: in the example with div, I get this:
<brade>  <ul><li><div>a</div></li> <li><div>b</div></li></ul>
<kin> well you get the idea of what I'm after :-)
<kin> what about inserting a block into a span?
<kin> what happens if you have <i>a|b</i>
<kin> and you insert a table?
<kin> <i>a</i><table></table><i>b</i>?
<kin> kaie, brade: ok try this
<kin> <span bold><span italics>a|b</span></span> and insert a table
<kin> it should split the spans all the way up to the containing block
<kin> and then insert the table there
<kin> if you did the same thing with a div
<kin> it just put the table in the div between the a and b
<brade> kin: if I type "ab" and then make them bold/italic, they are in same span
<kin> brade: I get 2 spans
<kin> in CSS mode
<brade> my build is from a day or two ago?
<brade> I'm in css mode
<kin> <span style="font-style: italic;"><span style="font-weight:
bold;">ab</span></span><br>
<kin> I just hit the  B and I buttons on the toolbar
<kin> and then typed "ab"
<brade> kin: I get <span style="font-weight: bold; font-style: italic;">ab</span>
<brade> kin: ah!
<brade> you clicked first!
<kin> yeah
<brade> kin: now I get that
<kin> there really shouldn't be a difference
<brade> I was doing it with a selection
<brade> kin, kaie: it works in my tree *without* kaie's patch
<kin> they all split up into the containing block right?
<brade> kin: I have 4 span tags now (2 before and 2 after the table)
<kin> brade: right that's what we want
<kin> now I'm wondeirng if harish's change will treat the  spans as if they were
a div
<kin> and just put the table in the span
<kin> the innermost span that is
<brade> kin, kaie: unless I didn't build enough; I don't see any problems
<brade> with the testcase above (bold, italic, type "ab", insert table btw ab)

Comment 17

[kinmoz]

Comment on attachment 127015 [details] [diff] [review]
Patch v1.0

sr=[e-mail]

Comment 18

[kinmoz]

Comment on attachment 127015 [details] [diff] [review]
Patch v1.0

Hmmm withdrawing sr=kin ... the change *is* causing NodeIsBlockStatic() to
return TRUE for spans, so the editor will think it's a block.

We'll need to poke around the editor to see what this could break.

Comment 19

[Kathleen :Brade]

Here is one testcase that doesn't work with this patch:
Steps:
 0) Ensure you have css styling in your prefs (to get the necessary span tags)
 1) click bold and italic icons on toolbar
 2) type: a[return]b[return]c
 3) select all
 4) click bulleted list icon
Expectation:
  * a
  * b
  * c
Result with patch:
  * a
    b
    c 

Comment 20

[harishd]

Looks like we need a better fix. Let me think about this a bit more.

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

Mass-removing myself from cc; search for 12b9dfe4-ece3-40dc-8d23-60e179f64ac1 or any reasonable part thereof, to mass-delete these notifications (and sorry!)