Closed Bug 42676 Opened 20 years ago Closed 10 years ago

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

Categories

(Core :: Selection, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: Chris244, Assigned: masayuki)

References

(Depends on 2 open bugs)

Details

(Keywords: testcase)

Attachments

(4 files, 13 obsolete files)

934 bytes, text/html
Details
1.05 KB, text/html
Details
26.63 KB, patch
Details | Diff | Splinter Review
6.59 KB, text/html
Details
<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.
Confirming 2000-06-16-12-M17 and changing to Selection.
Assignee: clayton → mjudge
Status: UNCONFIRMED → NEW
Component: Layout → Selection
Ever confirmed: true
QA Contact: petersen → elig
setting to m18
Target Milestone: --- → M18
i will bet these are classified in a different childlist than normal. i will 
peek at this.
Status: NEW → ASSIGNED
Keywords: correctness, nsbeta3
Target Milestone: M18 → M19
Keywords: nsbeta3
QA Contact: elig → BlakeR1234
*SPAM*: Changing the QA contact of all open/resolved Selection bugs from 
elig@netscape.com to BlakeR1234@aol.com.  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...
moving to future, adding helpwanted
Keywords: helpwanted
Target Milestone: M19 → Future
build 2001112104 win32

interestingly if I start the selection from above "One" I can select all text on
the page.
changing selection qa to tpreston.
QA Contact: blaker → tpreston
Keywords: testcase
Same with value "scroll" (1.8a6 NB/W2K, FF1.0/Linux). Moving to default.
Assignee: mjudge → selection
Status: ASSIGNED → NEW
OS: Windows NT → All
Priority: P3 → --
QA Contact: tpreston
Hardware: PC → All
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+
No longer blocks: 290932
*** Bug 290932 has been marked as a duplicate of this bug. ***
>>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.
Needs debugger to observe ongoing events.
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
Confirmed with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Martijn, would it be possible to have a better testcase which don't rely on Firebug? I cannot find its inspector component.
Assignee: selection → nobody
QA Contact: selection
Attached file 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?
Attachment #201945 - Attachment is obsolete: true
Attachment #10286 - Attachment is obsolete: true
Attached patch proof-of-concept patch (obsolete) — Splinter Review
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.
Duplicate of this bug: 347886
Duplicate of this bug: 458987
Summary: overflow:hidden causes selection problems → Can't drag to extend selection out of blocks with overflow:hidden/auto/scroll
Duplicate of this bug: 394664
Duplicate of this bug: 404929
Old and annoying bug with several duplicates. Could this get some attention for 1.9.2?
Flags: blocking1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Attached patch Patch v0.1 (obsolete) — Splinter Review
almost works, but there are some problems...
Assignee: nobody → masayuki
Attachment #342609 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch Patch v0.3 (obsolete) — Splinter Review
Works fine for me in simple cases, however, this should fail when the mouse cursor is on a different subtree.
Attachment #407281 - Attachment is obsolete: true
Attached patch Patch v1.0 (obsolete) — Splinter Review
maybe this is ok. I'll create many tests.
Attachment #407290 - Attachment is obsolete: true
Attached patch Patch v3.4 (obsolete) — Splinter Review
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.
Attachment #411647 - Attachment is obsolete: true
Attachment #411889 - Flags: review?(roc)
Olli is probably a better reviewer for this.
Comment on attachment 411889 [details] [diff] [review]
Patch v3.4

thank you, roc.

olli, would you review this?
Attachment #411889 - Flags: review?(roc) → review?(Olli.Pettay)
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.
Attached patch Patch v4.0 (obsolete) — Splinter Review
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.
Attachment #411889 - Attachment is obsolete: true
Attachment #413270 - Flags: review?(Olli.Pettay)
Attachment #411889 - Flags: review?(Olli.Pettay)
(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 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.
Attachment #413270 - Flags: review?(Olli.Pettay) → review-
Attached patch Patch v4.1 (obsolete) — Splinter Review
(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.
Attachment #413270 - Attachment is obsolete: true
Attachment #413358 - Flags: review?(Olli.Pettay)
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.
Attachment #413358 - Flags: review?(Olli.Pettay) → review+
(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.)
(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.
(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.
(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.
Attached patch Patch v4.2Splinter Review
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.
Attachment #413358 - Attachment is obsolete: true
Depends on: 535041
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
Attached file 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
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?
Depends on: 1120094
You need to log in before you can comment on or make changes to this bug.