729046 - nsHTMLEditUtils::CanContain and nsHTMLEditUtils::IsContainer should not use tag id ints

Status: NEW

(Core :: DOM: Editor, defect)

Description

[Henri Sivonen (:hsivonen)]

nsIParserService, nsHTMLTags, nsHTMLTagList and nsElementTable are going to go away.

nsHTMLEditUtils::CanContain and nsHTMLEditUtils::IsContainer need implementations that are independent of the old (or new) HTML parser.

Comment 1

[Henri Sivonen (:hsivonen)]

To fix this, one needs to see what tables these methods end up poking and arrange the same data in a reasonable way using nsGkAtoms.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

FWIW, these have definitions in the editing spec:

http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#allowed-children

But it's possible its definitions aren't good enough and would break stuff if Gecko switched to them.

Comment 3

[Mina Almasry]

Hi I think I can take this on. I'm fairly new here, though. Can you please elaborate on what exactly is needed here?

I understand CanContain returns true, if parent can contain the child, but false otherwise. So should I look at the link aryeh posted and implemented what's in that, or are there reservations about doing that?

What is IsContainer supposed to do?

Where do I start with writing a test for this bug?

Assign?

Comment 4

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

I have the feeling the algo in the editing spec is far too precise and "static". We need things more dynamic than that. We need to be able to tweak that algo in 5 seconds when a new element is added to html or an element that was in html is removed, as it happened recently. I am not even sure that algo is bug-free...

Comment 5

[(no longer active)]

(In reply to Daniel Glazman (:glazou) from comment #4)
> I have the feeling the algo in the editing spec is far too precise and
> "static". We need things more dynamic than that

Not sure what you mean.

> We need to be able to tweak
> that algo in 5 seconds when a new element is added to html or an element
> that was in html is removed, as it happened recently. I am not even sure
> that algo is bug-free...

Are you talking about the difficulty of updating the code when a new element is introduced?  I don't think 5 seconds is where I would put the bar, as the addition of new elements are quite a rare event.

Comment 6

[Mina Almasry]

Attachment: WIP patch

Henry,

I worked on the issue and came up with this patch. This patch builds and removes dependence on nsHTMLTags, which was the only dependence there is.

How is this? Do you want a test for this?

Comment 7

[Mina Almasry]

Attachment: patch

I don't know if you tried looking at the patch, but it was breaking your review tool. Here's one that should show up fine in your review tool.

Comment 8

[Mina Almasry]

Attachment: Test failures

I also ran the editor/libeditor/html/tests tests, and these are the failures I got, if that's useful.

Comment 9

[Henri Sivonen (:hsivonen)]

Comment on attachment 772340 [details] [diff] [review]
patch

>+GK_ATOM(whitespace, "whitespace")
>+GK_ATOM(entity, "entity")
>+GK_ATOM(doctypeDecl, "doctypeDecl")
>+GK_ATOM(markupDecl, "markupDecl")
>+GK_ATOM(instruction, "instruction")
>+GK_ATOM(blink, "blink")
>+GK_ATOM(eTemplate, "eTemplate")
>+
>+GK_ATOM(userdefined, "userdefined")

Apart from blink and template, these aren't actually used as element names and, therefore, don't actually need atoms, right?

Comment 10

[Mina Almasry]

Hey Henri!

They are in the kElements table here: http://dxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditUtils.cpp#l763

CanContain takes nsHTMLTags indicies into this array, and so it might take indicies that refer to those members of the array, no?

Which makes me notice. CanContain and IsContainer still take nsHTMLTags indicies as their arguments. Are you okay with that, or is part of this bug changing their function signature so that they take nsIAtom* arguments instead?

Comment 11

[Henri Sivonen (:hsivonen)]

(In reply to Mina Almasry from comment #10)
> Which makes me notice. CanContain and IsContainer still take nsHTMLTags
> indicies as their arguments. Are you okay with that, or is part of this bug
> changing their function signature so that they take nsIAtom* arguments
> instead?

The signatures should take nsIAtom* arguments. For CanContain, there's only one caller that first converts atoms to legacy tag enums:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#3483

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 772340 [details] [diff] [review]
patch

Marking this iteration r- considering the previous comments.

Comment 13

[Connor [:cojo]]

Is this is still in need of a patch? I had some questions on the best way to handle converting these methods.

> The signatures should take nsIAtom* arguments.

For |nsHTMLEditUtils::isContainer| [1] it looks like |aTag| is used to look up whether or not a given element has the property |mIsContainer|. The properties for each element are set in |kElements| [2] (|aTag| is used to look up in |kElements| whether an element has the property |mIsContainer|). 

If |nsHTMLEditUtils::isContainer| is changed to use to |nsIAtom*| is |kElements| still necessary? I don't know a whole lot about |nsIAtom*|. Is there a way to determine if |nsIAtom*| is a container or does |kElements| need to be modified so |nsIAtom*| can be used to do a look up in |kElements| (or can we get rid of |kElements| all together?).

[1] http://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditUtils.cpp#867
[2] http://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditUtils.cpp#637

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to Connor [:cojo] from comment #13)
> If |nsHTMLEditUtils::isContainer| is changed to use to |nsIAtom*| is
> |kElements| still necessary?

Some replacement structure for it seems necessary.

> I don't know a whole lot about |nsIAtom*|. Is
> there a way to determine if |nsIAtom*| is a container

There isn't.

> or does |kElements|
> need to be modified so |nsIAtom*| can be used to do a look up in |kElements|

Yes, this needs to be necessary.