42676 - Can't drag to extend selection out of blocks with overflow:hidden/auto/scroll

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Chris Hill]

<h1 style="overflow: hidden">One</h1>
<h1 style="overflow: hidden">Two</h1>
<h2>Three</h2>

When selection starts in an element with overflow:hidden, selection can't extend 
to any other elements.  

So if I start selection in One, I can't select Two or Three.  Similarly for Two.  
If I start selection in Three, I CAN select One and Two.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase given in bug

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Confirming 2000-06-16-12-M17 and changing to Selection.

Comment 3

[rubydoo123]

setting to m18

Comment 4

[mjudge]

i will bet these are classified in a different childlist than normal. i will 
peek at this.

Comment 5

[Blake Ross]

*SPAM*: Changing the QA contact of all open/resolved Selection bugs from 
[e-mail] to [e-mail].  After the many great years of service 
Eli has given to Mozilla, it's time for him to move on; he has accepted a 
position at Eazel.  We'll be sad to see him go, and I'll do my best to fill his 
spot...

Comment 6

[rubydoo123]

moving to future, adding helpwanted

Comment 7

[basic]

build 2001112104 win32

interestingly if I start the selection from above "One" I can select all text on
the page.

Comment 8

[Blake Ross]

changing selection qa to tpreston.

Comment 9

[Adam Hauner]

Same with value "scroll" (1.8a6 NB/W2K, FF1.0/Linux). Moving to default.

Comment 10

[Caleb]

Wow, this is such an annoying longstanding bug...

Confirmed: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050414 Firefox/1.0+

Comment 11

[Martijn Wargers (dead)]

*** Bug 290932 has been marked as a duplicate of this bug. ***

Comment 12

[sendmail.to]

>>overflow:hidden causes selection problems

Confirmed with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051104 Firefox/1.5

Also overflow: scroll; and overflow: visible; will cause selection problems.
Only overflow: visible; looks good.

Comment 13

[sendmail.to]

Attachment: Demonstrate the behaviour between different <p>'s

Comment 14

[I. Hela]

Attachment: Test case for mouse events

Needs debugger to observe ongoing events.

Comment 15

[I. Hela]

There is also problems when element has overflow:hidden in the realm of scripting and mouse events.

Confirmed  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1a3) Gecko/20060526 BonEcho/2.0a3

Comment 16

[Christian Ertl]

Confirmed with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Martijn, would it be possible to have a better testcase which don't rely on Firebug? I cannot find its inspector component.

Comment 18

[Martijn Wargers (dead)]

Attachment: testcase4

The mouse is captured when drag selecting inside overflow:scroll frames. Perhaps that mouse capturing should only happen when there is actually something to scroll in the frame?

Comment 19

[Uri Bernstein]

Attachment: proof-of-concept patch

This dispatches the even not only to the capturing view, but also to the original (outer) view (and to all views in between).
It mostly works, and doesn't regress textarea behavior.
The problem, however, is that the selection in this case flickers as the mouse is dragged, since when the capturing view handles the selection it truncates it to that view, and only later the parent view extends it back towards the pointer.

This perhaps could have been fixed using the selection batching mechanism, but it seems that it's virtually impossible to access it from nsViewManager (which doesn't know anything about selections, or even frames).

Ideas are welcome.

Comment 24

[Florian Quèze [:florian]]

Old and annoying bug with several duplicates. Could this get some attention for 1.9.2?

Comment 25

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

Attachment: Patch v0.1

almost works, but there are some problems...

Comment 26

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

Attachment: Patch v0.2

Comment 27

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

Attachment: Patch v0.3

Works fine for me in simple cases, however, this should fail when the mouse cursor is on a different subtree.

Comment 28

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

https://build.mozilla.org/tryserver-builds/[e-mail]-bug42676_v0.3/

Comment 29

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

Attachment: Patch v1.0

maybe this is ok. I'll create many tests.

Comment 30

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

Attachment: Patch v2.0

Comment 31

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

https://build.mozilla.org/tryserver-builds/[e-mail]-bug42676_v2.0/

Comment 32

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

Attachment: Patch v3.0

Comment 33

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

Attachment: Patch v3.1

Comment 34

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

Attachment: Patch v3.4

Roc, would you review this? Or Smaug is better reviewer?

This patch checks whether the frame under the mouse cursor is descendant of the selection root or not. If true, expands the selection to the frame, otherwise, uses the selection anchor (current behavior).

The automated test has two problems:

1. When it moves the mouse cursor to the iframe, the nsFrameSelection::HandleDrag() is called on the frame's shell, not capturing shell's. Therefore, the selection isn't changed by the mouse move event on the iframe.

2. When it moves the mouse cursor to the input field or textarea element and the containers are overflow: visible;, nsFrameSelection::HandleDrag() isn't called. I'm not sure where stops the processing.

But they shouldn't be problem in daily usage. And they are out of scope of this bug.

Comment 35

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

Roc:

Would you review this?

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Olli is probably a better reviewer for this.

Comment 37

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

Comment on attachment 411889 [details] [diff] [review]
Patch v3.4

thank you, roc.

olli, would you review this?

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 411889 [details] [diff] [review]
Patch v3.4

How does this work with (XBL) anonymous content?
Normally range objects' boundary points should be in the same (possibly anonymous) subtree.

Comment 39

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

Attachment: Patch v4.0

Good point, the previous patch sometimes returns another subtree's frame. I changed nsINode::GetSelectionRootContent. It should be return same subtree's content always.

However, XBL binding content breaks the selection by mouse moving (on the current trunk too). If I moves the mouse cursor to the XBL content, the selection handling is stolen by the subtree. I guess that nsIFrame::GetContentOffsetsFromPoint() returns another subtree's content.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2764
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2646

So, it should be another bug, I don't include the testcases for it.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #39)
> So, it should be another bug, I don't include the testcases for it.
Sure. It is not clear to me whether we even should fix selection handling in
several different subtrees. Or if it is fixed, how.
nsRange certainly doesn't handle that kind case by default.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 413270 [details] [diff] [review]
Patch v4.0

>+static nsIContent* GetRootForContentSubtree(nsIContent* aContent)
>+{
>+  NS_ENSURE_TRUE(aContent, nsnull);
>+  nsIContent* content = aContent;
>+  while (content) {
>+    nsIContent* parent = content->GetParent();
>+    if (!parent)
>+      break;
>+    PRUint32 childCount = parent->GetChildCount();
>+    if (childCount < 1)
>+      break;
>+    PRInt32 childIndex = parent->IndexOf(content);
>+    if (childIndex < 0 || PRUint32(childIndex) >= childCount)
>+      break;
>+    content = parent;
>+  }
>+  return content;
>+}
This is a bit slow, especially IndexOf.
You could do something like this:
  NS_ENSURE_TRUE(aContent, nsnull);
  nsIContent* stop = aContent->GetBindingParent();
  while (aContent) {
    nsIContent* parent = aContent->GetParent();
    if (parent == stop) {
      break;
    }
    aContent = parent;
  }
  return aContent;



> nsIContent*
> nsINode::GetSelectionRootContent(nsIPresShell* aPresShell)
So have you checked all the other callers of this method.
Can they handle the functional change?

And this change needs to be documented in nsINode.

Comment 42

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

Attachment: Patch v4.1

(In reply to comment #41)
> > nsIContent*
> > nsINode::GetSelectionRootContent(nsIPresShell* aPresShell)
> So have you checked all the other callers of this method.
> Can they handle the functional change?

Yes, because I created the method and it is used for getting the selection root content by each caller, so, the name and the new behavior do match.

However, nsFrameSelection::SelectAll() still doesn't use this method, I think we should change it. But I think it should be done in another bug in order to change clearly.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 413358 [details] [diff] [review]
Patch v4.1

>diff --git a/content/base/public/nsINode.h b/content/base/public/nsINode.h
>--- a/content/base/public/nsINode.h
>+++ b/content/base/public/nsINode.h
>@@ -798,17 +798,19 @@ public:
>    */
>   nsIContent* GetTextEditorRootContent(nsIEditor** aEditor = nsnull);
> 
>   /**
>    * Get the nearest selection root, ie. the node that will be selected if the
>    * user does "Select All" while the focus is in this node. Note that if this
>    * node is not in an editor, the result comes from the nsFrameSelection that
>    * is related to aPresShell, so the result might not be the ancestor of this
>-   * node.
>+   * node. Be aware that if this node isn't in same subtree as the selection
>+   * limiter of the nsFrameSelection, this returns the root content of the
>+   * bound contents.
I don't quite understand this comment.
"the root content of the bound contents"?

Please add some XBL tests.

Comment 44

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

(In reply to comment #43)
> >   /**
> >    * Get the nearest selection root, ie. the node that will be selected if the
> >    * user does "Select All" while the focus is in this node. Note that if this
> >    * node is not in an editor, the result comes from the nsFrameSelection that
> >    * is related to aPresShell, so the result might not be the ancestor of this
> >-   * node.
> >+   * node. Be aware that if this node isn't in same subtree as the selection
> >+   * limiter of the nsFrameSelection, this returns the root content of the
> >+   * bound contents.
> I don't quite understand this comment.
> "the root content of the bound contents"?

I meant the XBL subtree's root content.

> Please add some XBL tests.

When I move mouse on the XBL contents, the selection is aborted unexpectedly, therefore I didn't include the tests with XBL in the previous patch... (It's not a regression of my patch.)

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #44)
> When I move mouse on the XBL contents, the selection is aborted unexpectedly,
> therefore I didn't include the tests with XBL in the previous patch... (It's
> not a regression of my patch.)
But you did test XBL anyway. So it would be great to get at least some simple
XBL test here, if just possible.

Comment 46

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

(In reply to comment #45)
> (In reply to comment #44)
> > When I move mouse on the XBL contents, the selection is aborted unexpectedly,
> > therefore I didn't include the tests with XBL in the previous patch... (It's
> > not a regression of my patch.)
> But you did test XBL anyway. So it would be great to get at least some simple
> XBL test here, if just possible.

I found the bug, it's bug 451254.
https://bug451254.bugzilla.mozilla.org/attachment.cgi?id=414288
Try this testcase, you can check the drag aborting by the bug.

So, I have no idea of the profitable testcase under current behavior.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #46)
> So, I have no idea of the profitable testcase under current behavior.

I'd like to see some testcase, since the current behavior has been tested here,
and adding a testcase for that reduces the risk for unwanted regressions.

Comment 48

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

Attachment: Patch v4.2

This includes XBL contents, however, this test doesn't drag over the XBL binding contents because if we do so, the drag session will be broken. Instead, the test checks whether we can select the XBL content's children (<children/>) correctly or not.

Comment 49

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

http://hg.mozilla.org/mozilla-central/rev/ade8ba415789

Comment 50

[[e-mail]]

in Minefield/3.7a3pre 
When selection starts in an element with overflow!=visible, selection extends 
to other elements. but page doesn't scroll  

So if I start selection in element with overflow:hidden and move mouse out of page selection extends to start or the end of the page (this depends on where selection started, not the boundary, and if selection started in the middle it changes randomly with mouse move )
but page's scroll position isn't changed

Comment 51

[[e-mail]]

Attachment: testcase mouse events

with this patch scrollable element (element with overflow!=visible)
still captures mouse events, and doesn't allow other elements to process mouse
this patch forcibly extends selection but events have wrong rangeOffset and Parent

only when scrollable element can't be scrolled at (or is completely scrolled and mouse is far enough so that user definitely tries to extend selection not scroll!) it should leave events to other elements to allow selection extending

Comment 52

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

please file a new bug.

Comment 53

[[e-mail]]

filed https://bugzilla.mozilla.org/show_bug.cgi?id=552707 
please have a look at it 
it seems scrolling with selection doesn't work with iframes as well (noticed on the iframe in bug filling page) 
could you check if this happens because of this patch or something else?