Closed Bug 974309 Opened 10 years ago Closed 8 years ago

Contenteditable on page messes up with table

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: daniel.didusch, Assigned: bugs)

References

(
URL
)

Details

(Keywords: ux-consistency)

Attachments

(2 files, 3 obsolete files)

Shows the bug
4.76 KB, image/jpeg
Details
Fixes the IsEditable() logic for table cells. r=ehsan
5.79 KB, patch
bugs
: review+
Details | Diff | Splinter Review
Attached 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: 8 years ago

          status-firefox47:
          --- → fixed
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: 8 years ago → 8 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: