186118 - backspace over the end of a moz-user-select:none element breaks the DOM

Status: RESOLVED INVALID

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

Description

[Pierre Baillet]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20021126
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20021126

I'm working on a small snippet of HTML 
        <DIV class="AUTEURS">
            <DIV class="AUTEUR">
                <span class="NOM">nom1</span>
                <span class="PRENOM">prenom1</span>
                <DIV class="EMAIL">[e-mail]</DIV>
            </DIV>
            <DIV class="AUTEUR">
                <span class="NOM">nom2</span>
                <span class="PRENOM">prenom2</span>
                <DIV class="EMAIL">[e-mail]</DIV>
            </DIV>
        </DIV>
Pressing backspace just in front of nom2 shakes the DOM and gives:

<div class="AUTEURS">
<div class="AUTEUR"> <span class="NOM">nom1</span> <span
class="PRENOM">prenom1</span>
<div class="AUTEUR">[e-mail]<span class="NOM">nom2</span> <span
 class="PRENOM">prenom2</span>
<div class="EMAIL">[e-mail]</div>
</div>
</div>
</div>

Reproducible: Always

Steps to Reproduce:
Copy the HTML snippet in the source of the editor,
Press backspace in front of "nom2"
Look at the resulting DOM



Expected Results:  
. Probably mozilla editor should be aware of the -moz-user-select:none attribute
in the css and prevent people from removing the auteur element as easily

Comment 1

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

taking

Comment 2

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

This bug will be important for the implementation of templates in Composer.

Comment 3

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

cc:ing some people, nominating (the reporter is an embedder of Gecko needing
this extension).

Comment 4

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

Attachment: patch #1

This patch disallows the JoinNodes operation when a backspace key is pressed
right after a '-moz-user-select: none' element and when a delete key is pressed

right before such an element.

It also disallows the coalescing of similar inline nodes if one of them has
'-moz-user-select: none'.

It does NOT block the removal of such an element if it is contained in a
selection starting before it and ending after it.

Comment 5

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

Comment on attachment 109747 [details] [diff] [review]
patch #1

Joe, can you r= please ?

Comment 6

[Joe Francis]

Comment on attachment 109747 [details] [diff] [review]
patch #1

review notes:

Get rid of the changes to nsEditProperty.cpp & nsIEditProperty.h.  
Those changes are duplicates of changes I already checked in.

Throughout the diff get rid of tabs for indention and use spaces.

Please change these snippets in nsHTMLEditRules.cpp:
  {
    PRBool isSelectable;
    res = IsNodeSelectable(visNode, &isSelectable);
    if (NS_FAILED(res)) return res;
    if (!isSelectable)
      {
	*aCancel = PR_TRUE;
	return res;
      }
  }

 to:
  {
    PRBool isSelectable;
    res = IsNodeSelectable(visNode, &isSelectable);
    if (NS_FAILED(res)) return res;
    if (!isSelectable)
    {
      *aCancel = PR_TRUE;
      return res;
    }
  }


I have to admit I don't understand the intent of this patch.  For instance,
you have code in deletion case where we find we need to delete a br, hr, or
image, and ignore the deletion if these objects have select: none.  

It seems to me that it's fine to delete them.  Do you mean instead to
detect if their parent containers have the style?  I could understand
not wanting to partially delete the contents of a container that has
select: none.  However as written the patch will also not allow you to
delete an image or hr that has the select: none style directly on it.

Also, I see no corresponding code in the eNormalWS and eText cases of the
WillDeleteSelection().	If you have adjacent inlines, the first with
select: none, and the second without, and they both contain text, I think
you will be able to backspace right out of the 2nd one into the 1st one.

In the eOtherBlock and eThisBlock case I understand better what you are
doing (don't merge blocks if the other one has select: none).  I note that
you don't check the current block, only the other block.  So you are relying on
selection itself to prevent us from ever getting into that case, right?
One problem with the code in these cases is that you set up "otherNode"
but then never use it in the IsNodeSelectable() call.  I think you meant
to use otherNode instead of visNode?

In IsNodeSelectable(), I don't understand the use of htmlElement in the
while test.  Won't this cause you to stop checking as soon as you hit an
element?  Since the select attr doesn't inherit, it seems like you need
to look all the way up.  Certainly this is true of you wish to handle
the eText and eNormalWS case I mentioned above.

I've tried to make all the incest between the rules code and the editor
be from rules -> to editor.  So put IsNodeSelectable() in nsHTMLEditor
and call it from the rules code using mHTMLEditor->IsNodeSelectable(),
rather than calling from nsHTMLEditor using htmlrules->IsNodeSelectable().
I want the editor to make as few assumptions about the rules as possible.
Then you can get rid of all the mrules checks in nsHTMLEditorStyle.cpp

I think you also need the IsSelectableNode() test in RelativeFontChangeOnNode()
and RelativeFontChangeOnTextNode().

Comment 7

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/8)]

Current behavior matches with the other browsers'. Therefore, we should not change the behavior.