Contenteditable on page messes up with table

RESOLVED FIXED in Firefox 47

Status

()

defect
P5
normal
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: daniel.didusch, Assigned: bugs)

Tracking

({ux-consistency})

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

()

Attachments

(2 attachments, 3 obsolete attachments)

Posted image Shows the bug
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.107 Safari/537.36

Steps to reproduce:

Select text with and without contenteditable true.

<!Doctype html>
<html>
    <head>
        
    </head>
    <body>
        <div contenteditable="true" style="position: absolute; left: -999px; width: 100px; height: 100px;"></div>
        <table>
            <tr>
                <td>
                    Left
                </td>
                <td>
                    Right
                </td>
            </tr>
            <tr>
                <td>
                    Left
                </td>
                <td>
                    Right
                </td>
            </tr>
        </table>
    </body>
</html>


Actual results:

With state true tds of the table will be selected


Expected results:

Text selection, not the tds. Also tested in Chrome, Safari, Opera, IE - all work fine as expected.
You can test it here:
http://jsfiddle.net/danieldidusch/A784Q/
Was able to reproduce on Windows 8.1/7 x64 and Mac OS X 10.9 using Firefox 28 beta 4, latest Nightly.
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
OS: Windows 8.1 → All
Product: Firefox → Core
Hardware: x86_64 → All
Version: 27 Branch → Trunk
Severity: normal → minor
Component: Layout → Selection
Priority: -- → P5
Any solution so far?
Flags: needinfo?(mats)
It looks like this table selection behavior for contenteditable="true" is intentional:
http://hg.mozilla.org/mozilla-central/annotate/47c9418fbc28/layout/generic/nsSelection.cpp#l1605
mCellParent is assigned a few lines up: 1592, which is controlled by editor
per the comment.
Component: Selection → Editor
Flags: needinfo?(mats) → needinfo?(ehsan.akhgari)
I think the problem here is that we use the display selection check http://hg.mozilla.org/mozilla-central/annotate/47c9418fbc28/layout/generic/nsSelection.cpp#l1587 to tell whether we are in an editable region.  These days the right way to do this would be to call nsHTMLEditor::GetActiveEditingHost and verify that the content node is a descendant of that.
Flags: needinfo?(ehsan.akhgari)
This feature still is in version 35 of FF.
No one wants to resolve this (imho) feature?
I have no clue why this is "minor". This bug makes text selection impossible.
It's presented for many years on countless sites including:
Google Mail, vk.com (Russian social network - 70 million users every day), even html5test.com

I'm adding keyword "ux-consistency" because user got used to select text in such cases.
Severity: minor → normal
Keywords: ux-consistency
Mats: can you wire up the fix described in comment 5? We've gotten more external reports on this one. Thx!
Flags: needinfo?(mats)
Re: comment 5, it seems like the proper fix is to:
1. add a nsINode::IsInEditingMode method or some such (or an
   equivivalent nsContentUtils method) that figures out the right
   editor and calls GetActiveEditingHost on it and then returns
   true if the given node is a descendant
2. replace all checks of nsISelectionDisplay::DISPLAY_ALL with
   a call to said method
3. remove nsISelectionDisplay::DISPLAY_ALL
Re: comment 8, this work seems better suited for someone on
the DOM/Editor team.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #9)
> Re: comment 5, it seems like the proper fix is to:
> 1. add a nsINode::IsInEditingMode method or some such (or an
>    equivivalent nsContentUtils method) that figures out the right
>    editor and calls GetActiveEditingHost on it and then returns
>    true if the given node is a descendant
> 2. replace all checks of nsISelectionDisplay::DISPLAY_ALL with
>    a call to said method
> 3. remove nsISelectionDisplay::DISPLAY_ALL

I think #2 will change quite a bit more than desired (based on the usage count of nsISelectionDisplay::DISPLAY_ALL.) I'm going to post a less invasive patch and see if that does the trick.
Comment on attachment 8717402 [details] [diff] [review]
Fixes the IsEditable() logic for table cell contents per comment 5

Review of attachment 8717402 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!  Can you also add a test please?

::: layout/generic/nsSelection.cpp
@@ +1701,5 @@
> +    bool editable = false;
> +    nsHTMLEditor* editor = nullptr;
> +    nsPresContext* context = mShell->GetPresContext();
> +    if (context)
> +      editor = static_cast<nsHTMLEditor*>(nsContentUtils::GetHTMLEditor(context));

GetActiveEditingHost is defined on nsIHTMLEditor: <https://dxr.mozilla.org/mozilla-central/source/editor/nsIHTMLEditor.idl#557>.  So instead of casting like this, you can use do_QueryInterface() to get a correct nsCOMPtr<nsIHtmlEditor>.

@@ +1705,5 @@
> +      editor = static_cast<nsHTMLEditor*>(nsContentUtils::GetHTMLEditor(context));
> +
> +    if (editor) {
> +      nsINode* editorHostNode = editor->GetActiveEditingHost();
> +      editable = aNewFocus && editorHostNode && nsContentUtils::ContentIsDescendantOf(aNewFocus, editorHostNode);

Nit: aNewFocus has no bearing on whether we're in an editable node.  Plus, we already check for it in the beginning of TakeFocus(), so you don't need to check it again here.
Attachment #8717402 - Flags: review?(ehsan) → review-
New patch to address review comments and add tests.
Attachment #8717402 - Attachment is obsolete: true
Comment on attachment 8717605 [details] [diff] [review]
Fixes the IsEditable() logic for table cells per comment 13

Review of attachment 8717605 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: layout/generic/moz.build
@@ +192,5 @@
>      '../xul',
>      '/dom/base',
>      '/dom/html',
>      '/dom/xul',
> +    '/editor/libeditor',

This can now be removed.

::: layout/generic/nsSelection.cpp
@@ +76,5 @@
>  #include "mozilla/dom/ShadowRoot.h"
>  #include "mozilla/ErrorResult.h"
>  #include "mozilla/dom/SelectionBinding.h"
>  #include "mozilla/AsyncEventDispatcher.h"
> +#include "nsHTMLEditor.h"

Nit: nsIHTMLEditor.h

@@ +1698,5 @@
>      // BUT only do this in an editor
>  
>      NS_ENSURE_STATE(mShell);
> +    bool editable = false;
> +    nsPresContext* context = mShell->GetPresContext();

RefPtr<nsPresContext> please.

@@ +1700,5 @@
>      NS_ENSURE_STATE(mShell);
> +    bool editable = false;
> +    nsPresContext* context = mShell->GetPresContext();
> +    if (context)
> +    {

Nit: brace goes at the end of the previous line.

@@ +1703,5 @@
> +    if (context)
> +    {
> +      nsCOMPtr<nsIHTMLEditor> editor = do_QueryInterface(nsContentUtils::GetHTMLEditor(context));
> +      if (editor) {
> +        nsINode* editorHostNode = editor->GetActiveEditingHost();

nsCOMPtr<nsINode>

@@ +1713,1 @@
>      {

Same nit.
Attachment #8717605 - Flags: review+
(In reply to (Away 2/10-2/19) from comment #15)
> Comment on attachment 8717605 [details] [diff] [review]
> Fixes the IsEditable() logic for table cells per comment 13
> 
> Review of attachment 8717605 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: layout/generic/moz.build
> @@ +192,5 @@
> >      '../xul',
> >      '/dom/base',
> >      '/dom/html',
> >      '/dom/xul',
> > +    '/editor/libeditor',
> 
> This can now be removed.

I need it for nsIHTMLEditor.h, see below...

> ::: layout/generic/nsSelection.cpp
> @@ +76,5 @@
> >  #include "mozilla/dom/ShadowRoot.h"
> >  #include "mozilla/ErrorResult.h"
> >  #include "mozilla/dom/SelectionBinding.h"
> >  #include "mozilla/AsyncEventDispatcher.h"
> > +#include "nsHTMLEditor.h"
> 
> Nit: nsIHTMLEditor.h

I get 'no matching function for call to do_QueryInterface' errors when I fix this nit. I hope that's OK to keep.

I've made all the other changes requested and Try run looks good.
Attachment #8717605 - Attachment is obsolete: true
Attachment #8717667 - Flags: review+
Assignee: nobody → bugs
https://hg.mozilla.org/mozilla-central/rev/417d17932ff0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1268736
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I don't think reopening fixed bugs is the right way to handle additional fixes.
It'll be hard to track changes that way.  Please file a new bug instead,
or just use bug 1268736?
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Attachment #8755591 - Attachment is obsolete: true
Attachment #8755591 - Flags: review?(mats)
Depends on: 1426869
You need to log in before you can comment on or make changes to this bug.