552707 - While we're expanding selection by dragging, the selection root element should capture mouse events and all scrollable elements should be scrollable

Status: RESOLVED WORKSFORME

(Core :: DOM: Selection, defect)

Description

[[e-mail]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)

Reproducible: Always

Steps to Reproduce:
click on the element with overflow:hidden/auto/scroll and drag mouse out of the element
Actual Results:  
element with overflow:hidden/auto/scroll will capture mouse events (to handle scrolling) and don't allow element under  the mouse receive events and selection to extend, even if element isn't actually overflown 

Expected Results:  
if element isn't overflown (no active scrollbars) it should behave like element with overflow:visible
if element is overflown it should capture mouse events to handle scroll, untill there is no more space to scroll, after that it should allow other elements to receive mouse events and extend selection


in https://bugzilla.mozilla.org/show_bug.cgi?id=42676 changes were made to selection system so that it extends selection despite mouse being captured by scrollable element. this introduces number of regressions like selection disappearing on xbl elements and outer view doesn't scroll if selection was started within overflow!=visible element
seems that this patch breaks iframe scrolling with selection as well

Comment 1

[[e-mail]]

Attachment: testcase for mouse events

try this on mozilla1.9.3a1+ and on older version to see what changes were made

Comment 2

[[e-mail]]

two more things connected to scrollable elements and selection (should those be separate bugs?)
if selection part of scrollable view and some outer elements, dragging scrollbar moves scrollbar a little and then starts selection drag which is wrong

if selection is inside scrollable element which is overflown "select All" should extend it to that element only (usually it's only thing user needs) and next "select All" will select whole document

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

> seems that this patch breaks iframe scrolling with selection as well

Would you post the testcase? My patch shouldn't cross over the document boundary.
I don't find any problems on following testcase:
data:text/html,text<iframe src="data:text/html,text"></iframe>text

And would you separate a bug which is you cannot scroll the root scrollable view when you move mouse cursor to outside of the window? That works fine on ade8ba415789 build, it's not my bug.

Besides, the XBL problem is bug 451254, not a new regression.

This bug should take only the capture problem, I think.

Comment 4

[[e-mail]]

sorry
scrolling the root scrollable view when moving outside scrollable doesn't work on "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a4pre) Gecko/20100316 Minefield/3.7a4pre (.NET CLR 3.5.30729)", anywhere, so probably it's caused by something else
>Besides, the XBL problem is bug 451254, not a new regression.
selection disappearing wasn't happening when selectable elements were in the same anonymous tree where selection was started (add-on i am developing was using it)

don't you think that selection should go outside scrollable element only once it's finished scrolling? 

I think selection shouldn't be able to extend out of the element if mouse is captured

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #4)
> don't you think that selection should go outside scrollable element only once
> it's finished scrolling? 

I don't think so, but when the cursor is near the edge of scrollable element, it should cause the scrolling, probably.

> I think selection shouldn't be able to extend out of the element if mouse is
> captured

It's interesting. Could the change break some Web apps? Even if it's wrong, I think that the selection should be able to be extended to outside the scrollable elements. So, if we need to change the behavior, the root element always should capture and we should implement new autoscrolling system during selecting.

Comment 6

[[e-mail]]

(In reply to comment #5)
> It's interesting. Could the change break some Web apps? 

not many probably, since people know those things are buggy in most browsers and don't rely on them to much, but anyway if mouse is captured it shouldn't interact with anything but capturing element otherwise capturing is pointless

> Even if it's wrong, I think that the selection should be able to be extended
> to outside the scrollable elements

yes, not being able to do that is really annoying especially when no scrollbars are visible

but there are several problems to solve to make it work properly
1)selection jumps while element is scrolled 
2)selection extends to the whole page while selecting in a fixed position element, and jumps all over page if it's scrollable
3)if selection comes over the scrollable element from outside it doesn't capture mouse and start scrolling, so to select scrolled out things one must start selecting anew
4)selection extends onto the fixed position elements selecting, say, document to the position of the fixed element and the text at the and of that element, which is meaningless
5)while selecting-scrolling to the down selections focus goes to the documents start
generally focus node should never go far from the mouse, or select noncontinous 

I thought waiting until scrolling is finished in combination with tracking how fast and how far mouse moved could solve 1 and 2. do you have other ideas?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

capture stating at nsFrame::HandlePress():
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1938

auto scrolling target is specified by nsFrame::HandleDrag():
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2288

actual scrolling code is PresShell::ScrollFrameRectIntoView():
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#4118

So, I think:
* nsFrame::HandlePress() start to capture the root element of the document.
* nsFrame::HandleDrag() sends a frame which is under the cursor.
* PresShell::ScrollFrameRectIntoView() should have a new flag like |SCROLL_ON_FRAME_EDGE| and if the cursor near a scrollable frame edge, the scrollable frame should be targeted.

By this approach, users can scroll all scrollable frame without mouse wheel during selecting. It's better than now, probably.

I don't think the "jump" is important problem because Google Chrome and IE have same issue. That isn't the regression, and we need more discussion and works.

Comment 8

[[e-mail]]

>* nsFrame::HandlePress() start to capture the root element of the document.
capture needed for scrolling and to prevent selection extending while scrolling
(see comment on line 1938) so why capture root? 

>* PresShell::ScrollFrameRectIntoView() should have a new flag like
>|SCROLL_ON_FRAME_EDGE| and if the cursor near a scrollable frame edge, the
>scrollable frame should be targeted.
 what StartAutoScrollTimer does?

> By this approach, users can scroll all scrollable frame without mouse wheel
> during selecting. It's better than now, probably.

on ff 3.6 users can scroll nested scrollable frames, without mouse wheel, during selecting in 3.7a2 and above this don't work anymore, so it would be good to find patch which broke this 
 
> I don't think the "jump" is important problem because Google Chrome and IE have
> same issue. That isn't the regression, and we need more discussion and works.
opera have the same problem too, but ff's selection was less jumpy, and ff must be better than others of course

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #8)
> >* nsFrame::HandlePress() start to capture the root element of the document.
> capture needed for scrolling and to prevent selection extending while scrolling
> (see comment on line 1938) so why capture root? 

I don't think so with my idea.

> >* PresShell::ScrollFrameRectIntoView() should have a new flag like
> >|SCROLL_ON_FRAME_EDGE| and if the cursor near a scrollable frame edge, the
> >scrollable frame should be targeted.
>  what StartAutoScrollTimer does?

StartAutoScrollTimer calls PresShel:ScrollFrameRectIntoView() via nsTypedSelection::DoAutoScroll().

> > By this approach, users can scroll all scrollable frame without mouse wheel
> > during selecting. It's better than now, probably.
> 
> on ff 3.6 users can scroll nested scrollable frames, without mouse wheel,

I don't think so, how do I scroll the inner scrollable element of the selection starting element?

Comment 10

[[e-mail]]

(In reply to comment #9)
> I don't think so with my idea.
then probably call to SetCapturingContent will not be needed at all

> StartAutoScrollTimer calls PresShel:ScrollFrameRectIntoView() via
> nsTypedSelection::DoAutoScroll().
shouldn't it be used instead of PresShell::ScrollFrameRectIntoView()
to keep scroll animation?

> I don't think so, how do I scroll the inner scrollable element of the 
> selection starting element?
i didn't realize you meant this case as well,
it will be rally better than now

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v1.0

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-b3bf71505fb6/

test builds.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v2.0

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v3.0

I still need to work more something...

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v4.0

this patch almost works fine for me.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

The patch makes new oranges only on Mac...

> s: talos-r3-leopard-008
> 35730 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug493251.html | Wrong number events (18) - got 0, expected 1
> Bug 565245 - Intermittent failure in test_bug493251.html | Wrong number events (21) - got 1, expected 2 35733 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug493251.html | Wrong number events (21) - got 0, expected 1
> 35736 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug493251.html | Wrong number events (21) - got 0, expected 2
> 35940 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug545268.html | Wrong number events (18) - got 0, expected 1
> 35943 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug545268.html | Wrong number events (21) - got 0, expected 1
> 35946 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug545268.html | Wrong number events (21) - got 0, expected 2

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

I fixed comment 16's issue on my local tree. The cause is an unknown click event handling bug on current trunk. I'll post a new patch with a lot of testcases and I'll request review for it.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v5.0

Here are patched builds:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-38e5cbc6be19/

First, I should ask to jboriss about the new behavior.

When press mouse button on a content, Gecko set a capture content to a selection root content (In most cases, it's a root element of the document. When mouse button is pressed on editor, it's a root element of the editor). By this behavior, the selection cannot be expanded to outside of the capturing content. And when mouse cursor is moved to outside of the capturing content, the capturing content will be scrolled (same as current behavior).

But by this change, user cannot scroll any scrollable sub frames. Against that, this patch adds a new feature. When the user moves mouse cursor on the edge of scrollable sub frames, the frame will start scrolling. Therefore, users can expand selection to any position even if the point is hidden by scrollable frame.

Note that even if the mouse cursor is moved to edge of scrollable frame, if the scrollable frame is another document's frame (e.g., iframe), or out of the capturing content, or in another selection root (e.g., editor), the subframe will not be scrolled.

Comment 19

[[e-mail]]

this works great, thanks
i think only two things could be improved a little

 since scrollbars are perceived as a part of frame dragging over them should also scroll. (now in case when frame has a horizontal scrollbar vertical scroll stops when mouse is over it)
 scrolling area could be just a few pixels bigger than frame, enough to be able to scroll keeping mouse on the border of the frame or beneath it (this would be useful since aiming at the border is easier than aiming at some place inside the frame)

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #19)
>  since scrollbars are perceived as a part of frame dragging over them should
> also scroll. (now in case when frame has a horizontal scrollbar vertical scroll
> stops when mouse is over it)

I was thinking about that. First, it may be possible and easy. However, there are a couple of problems. One is whether the scrollbar is included in the "edge" or not. If it's included, the visual edge width of the edges are different between top or left edge and bottom or right edge. If they are not included, the right and bottom edges are thicker than others.

>  scrolling area could be just a few pixels bigger than frame, enough to be able
> to scroll keeping mouse on the border of the frame or beneath it (this would be
> useful since aiming at the border is easier than aiming at some place inside
> the frame)

I don't have good idea for this. The mousemove event is handled on a frame which is at the mouse cursor position. So, the scrollable area cannot catch the event without capturing. I think that your idea is better behavior, I agree. But It should be separated to new bug. I cannot fix it easily right now.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #20)
> (In reply to comment #19)
> >  scrolling area could be just a few pixels bigger than frame, enough to be able
> > to scroll keeping mouse on the border of the frame or beneath it (this would be
> > useful since aiming at the border is easier than aiming at some place inside
> > the frame)
> 
> I don't have good idea for this. The mousemove event is handled on a frame
> which is at the mouse cursor position. So, the scrollable area cannot catch the
> event without capturing. I think that your idea is better behavior, I agree.
> But It should be separated to new bug. I cannot fix it easily right now.

If the curosr is on the border, the frame is the handler. I said about the cause that the mouse cursor is at outside of the frame but it's cross to the frame.

So, I think that if we shouldn't prefer the padding box, we can use border box. Then, the scrollbars are included the edge. But I'm not sure which is most natural behavior about most users...

Comment 22

[[e-mail]]

(In reply to comment #20)
> I don't have good idea for this. The mousemove event is handled on a frame
> which is at the mouse cursor position. So, the scrollable area cannot catch the
> event without capturing. I think that your idea is better behavior, I agree.
> But It should be separated to new bug. I cannot fix it easily right now.
probably if frame already captures mouse for scrolling it could hold capture a little bit longer, that way it wouldn't start scroling too soon (before mouse reached frame) and won't stop when user moves mouse farther to scroll quicker 


>So, I think that if we shouldn't prefer the padding box, we can use border box.
>Then, the scrollbars are included the edge. But I'm not sure which is most
>natural behavior about most users...
now firefox (and most other programs on windows) do not scroll when mouse is inside frame (old behavior in fact was encouraging to carelessly move mouse as far as possible). therefore people would tend to move mouse to the outer border of frame 

so, I think the border box, and several pixels out of it before stoping scroll
would be ideal

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #22)
> (In reply to comment #20)
> > I don't have good idea for this. The mousemove event is handled on a frame
> > which is at the mouse cursor position. So, the scrollable area cannot catch the
> > event without capturing. I think that your idea is better behavior, I agree.
> > But It should be separated to new bug. I cannot fix it easily right now.
> probably if frame already captures mouse for scrolling it could hold capture a
> little bit longer, that way it wouldn't start scroling too soon (before mouse
> reached frame) and won't stop when user moves mouse farther to scroll quicker 

That is the capturing content's behavior, not inner frame behavior. Any inner frame which is descendant of the capturing content cannot catch mousemove event when the cursor is outside of it.

Note that if script captures a content at mousedown event, the content will be root of the selection, so, users cannot expand the selection to outside of it *by the dragging*. And the content's scrollable view is out-most scrollable frame at this time rather than inner frame.

> >So, I think that if we shouldn't prefer the padding box, we can use border box.
> >Then, the scrollbars are included the edge. But I'm not sure which is most
> >natural behavior about most users...
> now firefox (and most other programs on windows) do not scroll when mouse is
> inside frame (old behavior in fact was encouraging to carelessly move mouse as
> far as possible). therefore people would tend to move mouse to the outer border
> of frame 

Such application doesn't have two or more scrollable views in a window. And dragging for selection cannot cross the window boundary. So, we should prefer dragging behavior for D&D with ActiveX rather than the drag for selection behavior.

At D&D with ActiveX, dragging data can cross window/application boundary. If application supports scrolling such as wordpad at that case, the scrolling is started at on the edge of its contents.

> so, I think the border box, and several pixels out of it before stoping scroll
> would be ideal

Um, wordpad scrolls on its scrollbars too but the "edge width" is comported from padding-box. I guess that it's better behavior. I'm going to change the behavior in next patch.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v5.1

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-08743db83359/

Okay, this updates the edge definition. They includes border and scrollbars but the thickness is 32px from the padding-box. I.e.,

+----------------------------------------------------------------+
|                     border-box                                 |
|  +----------------------------------------------------------+  |
|  |                                                       +--+  |
|  |                                                       +--+  |
|  |                                                       |  |  |
|  |                                                       |  |  |

                                                             ^- scrollbar
|<------->|  Left (or Top) edge is border + 32px.

Right (or Bottom) edget is border + scrollbar + 32px |<--------->|

Comment 25

[[e-mail]]

(In reply to comment #24)
I've been using this build for some time and it is very good indeed
I think this definition of edges is better and more natural 

i think only remaining issue with scrollable frames is that dragging scrollbar of the fully selected frame starts drag of selected text.
do you know if there is bug for that

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #25)
> i think only remaining issue with scrollable frames is that dragging scrollbar
> of the fully selected frame starts drag of selected text.
> do you know if there is bug for that

That's bug 491079.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

I filed bug 590819 for comment 16's issue. That should be fixed first, this may need long time for review...

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v5.1.1

I should ping jboriss...

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 471395 [details] [diff] [review]
Patch v5.1.1

Um, jboriss hasn't been replied...

I don't know other good UI reviewer and it seems that this patch doesn't need ui-review in strict rules.

Smaug, would you review this patch? If you think that this must be reviewed by other additional reviewers, would you suggest them?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Renominating for blocker because this bug was marked as blocking- before I started to work on this bug.

The main issue of this bug is that users can change selection by dragging even outside of capturing element. That was expected behavior, but also a regression of bug 42676 (so, the approach of bug 42676 had some wrong points).

My patch of this bug makes a selection root element always capture. Therefore, this fixes the above problem.  However, the change has a problem. That is users cannot scroll any scrollable subframes (like a div with overflow: auto;) during dragging.

Against the new problem, this patch provides a new scrollable way. That is, when users move mouse cursor to edge of scrollable subframe, the patch scrolls the subframe continuously. However, on the selection root element (i.e., capturing element), this new behavior doesn't work. So, users can make it scroll by same operation as before (move cursor outside of the capturing element). Additionally, users *also* can make subframes scroll by new operation (move cursor on its edge).

I think that this is NOT an important change but if this will be fixed, some pages become more accessible if users want to expand selection by dragging.

Comment 31

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

(In reply to comment #30)
> That is users
> cannot scroll any scrollable subframes (like a div with overflow: auto;) during
> dragging.

vs.

> Against the new problem, this patch provides a new scrollable way. That is,
> when users move mouse cursor to edge of scrollable subframe, the patch scrolls
> the subframe continuously.
So I don't quite understand the difference.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

User can scroll selection root's scrollable element (typically, <body> or <input> or <textarea>) when he/she moves the mouse cursor to outside of the element. This is implemented by "capture" mechanism. However, this cannot scroll any other scrollable elements in the selection root.

This patch provides the new auto scroll function when user moves mouse cursor to the "edge" of a scrollable element.

I.e., users can scroll <body> element by same action as current trunk. On the other hand, he/she can scroll any other scrollable frame (except another document's scrollable element and another selection root's scrollable frame like <input>) when hs/she moves the mouse cursor on the edge.

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

roc:

would you give some feedback for this bug?
# smaug requested feedback? to you.

Comment 34

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

I really don't know what to say. Bring it up in dev.apps.firefox? or ask Alexander Limi for input?

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 471395 [details] [diff] [review]
Patch v5.1.1

Thank you, roc. Requesting ui-review to Alexander Limi.

See comment 18, comment 24, comment 30 and comment 32 for the detail.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

I reposted the patch to tryserver for ui-review.
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-6f0487a62255/

Comment 37

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 471395 [details] [diff] [review]
Patch v5.1.1

(I'm out of the office this month, maybe faaborg can take a look instead?)

Comment 38

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 471395 [details] [diff] [review]
Patch v5.1.1

I'm giving this a ui-review+ based on the described functionality, however I do have a question that I'm not sure about:

Will this change potential lead to situations where during selection an area begins to scroll, and both the user and the Web developer did not anticipate that scrolling to occur?  I also wasn't entirely clear on what the other browsers are doing, and if this change makes us more or less consistent with their behavior (not that that would necessarily effect the review, but is useful context).

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Thank you, Alex.

(In reply to comment #38)
> Will this change potential lead to situations where during selection an area
> begins to scroll, and both the user and the Web developer did not anticipate
> that scrolling to occur?

I think that the possibility isn't zero. But I don't think that the new behavior will block something users want to do. And also, web developers cannot stop scrolling during selection on current design too.

> I also wasn't entirely clear on what the other
> browsers are doing, and if this change makes us more or less consistent with
> their behavior (not that that would necessarily effect the review, but is
> useful context).

IE8 is same as our current behavior, i.e., user can scroll only nearest scrollable area from selection start.

Chrome users can scroll only all ancestor scrollable area from selection start when they move cursor to outside of each scrollable area.

Comment 40

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

I'll try to review this tomorrow, but since this is pretty much all layout/
code, and a quite huge patch, a layout peer should look at this too, IMO.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #40)
> I'll try to review this tomorrow, but since this is pretty much all layout/
> code, 

ping...

> and a quite huge patch, a layout peer should look at this too, IMO.

Yeah, I agree.

Comment 42

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

Attachment: Few minor comments

I need to still review nsFrame.cpp (so most of the code).
But as I said, some layout peer really needs to look at this too.

Comment 43

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

Attachment: Few minor comments

Oops, this one.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Thank you, smaug.

> Perhaps GetMouseDownFrameSelection();
> Or GetDraggingFrameSelection()

"dragging" shouldn't be used for this bug because there is another dragging operation. I think we should use "dragging" for it.

> +    // body's bounding client rect depends its scroll position.
> +    var body = aTarget.ownerDocument.body;
> +    if (body == aTarget) {
> +      left += aTarget.scrollLeft;
> +      top += aTarget.scrollTop;
> +    }
> +
> So what does the spec say about boundingclientrect in this case?
> Does Gecko do the right thing, or should we change something in
> the implementation and not in .js.

getBoudningClientRect() isn't standard, it's IE's spec.

And IE 8 returns same value as Gecko. So, the our getBoundingClientRect() shouldn't be changed.

Comment 45

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

getBoundingClientRect is definitely in a standard. See http://dev.w3.org/csswg/cssom-view/#the-getclientrects-and-getboundingclient

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #45)
> getBoundingClientRect is definitely in a standard. See
> http://dev.w3.org/csswg/cssom-view/#the-getclientrects-and-getboundingclient

Oh, thank you.

And I have some additional information. IE8 always returns 0, 0 for body element in quirks mode. But Gecko and IE8 standards mode return the relative position from the viewport. So, we don't have problem for the implementation except the compatibility with IE's quirks mode.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 471395 [details] [diff] [review]
Patch v5.1.1

Sigh... This patch cannot pass the new tests on current trunk build. I'll check the cause.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v5.2

Testing on tryserver.

I rewrote the tests. This is easier to maintain than the old one.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 496537 [details] [diff] [review]
Patch v5.2

changed only your reviewed point and new tests.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 496537 [details] [diff] [review]
Patch v5.2

And Smaug wanted another reviewer of layout peer. Roc, would you review this? The detail of this patch is written at comment 18, comment 24, comment 30 and comment 32.

Comment 51

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

Why would we want to block on this?

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #51)
> Why would we want to block on this?

Current trunk build can change selection of outside of capturing content. That is a regression of bug 42676 (The approach of the patch of the bug is wrong).
# Current trunk build sets a nearest scrollable element as capturing content and handling mousemove events for outside of the capturing content for expanding selection.

But fixing for it, I need to provide new way for scrolling sub frames which are not capturing content because current build can scroll it. So, the regression fix and implementing new behavior must be fixed same time.

The new behavior was already reviewed by faarborg.

Comment 53

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

How serious is this bug though? I don't know of any duplicates filed. You say that IE8 is the same as our current behavior. I really don't think this needs to block.

Comment 54

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

(And I think there is significant risk.)

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #53)
> How serious is this bug though?

I said that in comment 30 at requesting the blocker flag. I don't think this is serious.

> I don't know of any duplicates filed. You say
> that IE8 is the same as our current behavior. I really don't think this needs
> to block.

I meant that the behavior means the scrolling target is the nearest scrollable element from mousedown element only.

Note that on IE8, when script captures the mouse events, users cannot select any contents. Chrome doesn't support mouse capture on current released build. Furthermore, Opera is same as Fx3.6.

                       Fx3.6  trunk   My patch  IE8  Chrome  Opera

Can select
outside of nearest       X      O        O       O     O       X
scrollable element?

Can select outside of
mouse capturing element? N/A    O        X       X     N/A     N/A

Can scroll the nearest
scrollable eleemnt?       O     O        O       O      O       X

Can scroll the body
element?                  O     O        O       O      O       O

Can scroll nested
ancestor scrollable       O     O        O       O      O       X
element?

Can scroll another
scrollable element?       X     X        O       X      X       X


* setCapture is supported only on trunk and IE8.

So, we don't need to worry about the compatibility with other browsers. But the patch improves:

1. User can scroll any scrollable elements during selection.
2. User can scroll non-root scrollable elements on its edge. This is easier to scroll current all browser's behavior.

(In reply to comment #54)
> (And I think there is significant risk.)

Therefore, I have started working on this bug since July and requesting the review since August. But that is still under review process :-(


I'm not sure this should be a blocker even fix the unexpected behavior (can select outside of the capturing element's contents) which was regressed by bug 42676. However, this should be wanted on Fx4 because this fix prevents the compatibility issue (whether the outside contents of capturing mouse events are selectable or not) with post-Fx4 builds during capturing mouse events.

Comment 56

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

I'm sorry about the lengthy review process but I still don't think we should block.

Comment 57

[[e-mail]]

any chance for this landing before all patches get horribly outdated?
test builds were working really well, it's a pity to loose such a great work;(

Comment 58

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

+const kTimeoutTimeForScrollingTest = 30000; // should be enough long for preventing random failure
+const kTimeoutTimeForNonScrollingTest = 1000;

I don't think you should use this timeout. Instead, let the built-in mochitest timeout take care of timeouts.

+    if (++aTest.retryCount >= 10) {
+      ok(false, aTest.description + ": failed to run test");
+      runTests();
+      return;
+    }
+    // retry with delay
+    setTimeout(doTest, 100, aTest);

Similarly here I think we can just rely on the mochitest timeout. And the setTimeout here should just use 0 as the delay.

+  // If the specified selection root content is captureing content,

capturing

+  // If the specified selection root content is captureing content,
+  // that might be different as computed selection root.  Then, we should

I think you mean "different from the computed selection root".

+  // If the specified selection root content is captureing content,
+  // that might be different as computed selection root.  Then, we should
+  // replace aSelectionRoot with its computed selection root.

Can you give an example of when this would happen?

+        // If aSelection root isn't null, find a scrollable frame which
+        // selection root is same as aSelectionRoot.

"If aSelectionRoot"

"whose selection root is the same as"

+static nsIScrollableFrame*
+FindNearestScrollableFrameForSelection(nsIFrame* aFrame,
+                                       nsIContent* aSelectionRoot = nsnull)

I think we need a good comment explaining what this function does.

+  // a nearest scrollable view should be caputred the mouse because each

"captured"

Also, it's a scrollable frame, not a scrollable view.

+  // scrollable frame except the nearest one isn't needed to scroll during
+  // selection.

"doesn't need"

+  // If something else is already capturing the mouse, current selection root

"the current selection root"

+    return NS_OK;  // The capture was cancelded.

"canceled"

+      // is on its edge because selection root frame will be scroll when the

"will be scrolled"

+        aFrameSelection->StartAutoScrollTimer(
+          scrollableFrame->GetScrolledFrame(), scrollTo, 30);

Pull out "30" into a named constant.

+  aFrameSelection->StartAutoScrollTimer(scrolledFrame, scrollTo, 30);

Use the same constant here.

+  const char* kPrefName_EdgeWidth =
+    "layout.selection.drag.autoscroll.inner_frame.edge_width";

static const char kPrefName_EdgeWidth[] =

+  // frmae.

frame

+  nscoord edgeApp = nsPresContext::CSSPixelsToAppUnits(edgePixel);
+  nscoord onePixel = nsPresContext::CSSPixelsToAppUnits(1);

Perhaps we should use device pixels here for the zoomed case?

+  const char* kPrefName_ScrollAmount =

static const char kPrefName_ScrollAmount[] =

+      aScrollTo.x = scrollPosition.x + scrollPort.width + scrollAmountH;

Why + scrollPort.width?

+  if (ptInScrollableFrame.x < scrollPort.x + scrollPort.x + edgeH) {

Why are you multiplying scrollPort.x by 2 here?

+}
+NS_IMETHODIMP

Need newline

+  nsRefPtr<nsFrameSelection> fs = FindDraggingFrameSelection(&targetFrame);
+  if (!fs) {
+    fs = GetFrameSelection();

Why do we have to handle the !fs care here in HandleRelease?

+  // First, stop expanding selection (if it's doing)

"First, stop expanding selection if necessary"

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #58)
> +const kTimeoutTimeForScrollingTest = 30000; // should be enough long for
> preventing random failure
> +const kTimeoutTimeForNonScrollingTest = 1000;
> 
> I don't think you should use this timeout. Instead, let the built-in mochitest
> timeout take care of timeouts.

The timeouts are a part of tests. Don't you misunderstand about the meaning of timeouts? Or do you suggest to use other method to catch the timeouts of each step?

> +  // If the specified selection root content is captureing content,
> +  // that might be different as computed selection root.  Then, we should
> +  // replace aSelectionRoot with its computed selection root.
> 
> Can you give an example of when this would happen?

When a web page captures the mouse event by non-selection root element, then, we need to replace aSelectionRoot to the computed selection root for checking below.

> +  nscoord edgeApp = nsPresContext::CSSPixelsToAppUnits(edgePixel);
> +  nscoord onePixel = nsPresContext::CSSPixelsToAppUnits(1);
> 
> Perhaps we should use device pixels here for the zoomed case?

Absolutely!

> +      aScrollTo.x = scrollPosition.x + scrollPort.width + scrollAmountH;
> 
> Why + scrollPort.width?

Becasue nsTypedSelection::DoAutoScroll() scroll until the point becomes visible. So, it's not the top-left of the scrolled frame.

> +  nsRefPtr<nsFrameSelection> fs = FindDraggingFrameSelection(&targetFrame);
> +  if (!fs) {
> +    fs = GetFrameSelection();
> 
> Why do we have to handle the !fs care here in HandleRelease?

This is fallback for the old code, I agree with you. Probably, this isn't needed now.

Comment 60

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

(In reply to comment #59)
> The timeouts are a part of tests. Don't you misunderstand about the meaning of
> timeouts? Or do you suggest to use other method to catch the timeouts of each
> step?

Maybe I do misunderstand.

+    gTimer = setTimeout(checkResult, kTimeoutTimeForScrollingTest, true);

The idea of this timer is that we expect a scroll to happen, and when it does happen onScroll will be called, so this timeout is just to catch cases wher it never happens --- right? If so, we should be able to get rid of this timeout and just let the test timeout normally because onScroll is never called with the right scroll position.

+      gTimer = setTimeout(checkResult, kTimeoutTimeForNonScrollingTest, true);

The idea here is that we wait for kTimeoutTimeForNonScrollingTest to ensure that no scrolling happens, right? If we wait for too short or too long, the test should still pass. So I guess this setTimeout is OK, although it will make the test slow :-(.

> > +      aScrollTo.x = scrollPosition.x + scrollPort.width + scrollAmountH;
> > 
> > Why + scrollPort.width?
> 
> Becasue nsTypedSelection::DoAutoScroll() scroll until the point becomes
> visible. So, it's not the top-left of the scrolled frame.

I see. Instead of calling this variable aScrollTo, let's call it aScrollIntoView so it's clear that this is not the scroll position, just a point that will be scrolled into view.

Comment 61

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

Are you perhaps updating the patch?
Do you know how IE9 behaves?

(Sorry about this taking ages.)

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #61)
> Are you perhaps updating the patch?

Yeah, I'll update ASAP. But the priority of this bug is lower than other my current work.

> Do you know how IE9 behaves?

IE9 scrolls only the nearest scrollable content from mousedown element when mouse cursor moves outside of the content. But it's not scrollable if it's in fixed element.

Comment 63

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

Comment on attachment 496537 [details] [diff] [review]
Patch v5.2

I assume a new patch is coming.

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v5.3

Comment 65

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

Comment on attachment 537694 [details] [diff] [review]
Patch v5.3

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

::: layout/generic/test/window_selection_scrolling.html
@@ +1322,5 @@
> +];
> +
> +var gTimer;
> +
> +const kTimeoutTimeForScrollingTest = 30000; // should be enough long for preventing random failure

This is no longer used.

@@ +1408,5 @@
> +      runTests();
> +      return;
> +    }
> +    // retry with delay
> +    setTimeout(doTest, 100, aTest);

As I said in comment #58, you should just use 0 as the timeout here and don't limit the retryCount. Instead rely on the mochitest harness timeout to stop the test.

Comment 66

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch 5.4

Thank you, roc.

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v5.4.1

Moving the new test to mochi-chorme due to new rule (must not use enablePrivilege('UniversalXPConnect') in new tests).

Comment 68

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

Comment on attachment 541980 [details] [diff] [review]
Patch v5.4.1

Is this really the right patch?

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v5.4.1

Oops, I'm sorry.

Comment 70

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

Comment on attachment 543010 [details] [diff] [review]
Patch v5.4.1


>+static nsIContent*
>+GetContentToCaptureForSelection(nsIContent* aSelectionRoot)
>+{
>+  if (!aSelectionRoot->IsInNativeAnonymousSubtree()) {
>+    return aSelectionRoot;
>+  }
>+  return aSelectionRoot->GetBindingParent();
>+}
Why this behavior?
Why not returning aSelectionRoot->FindFirstNonNativeAnonymous()?

Does this patch change selection handling when there is 
generated content? (:before and :after)
I'll upload a testcase.


>+nsFrame::HandleDrag(nsPresContext* aPresContext,
>+                    nsGUIEvent*    aEvent,
>+                    nsEventStatus* aEventStatus)
>+{
>+  nsFrame* target;
>+  nsFrameSelection* fs =
This should be probably nsRefPtr<nsFrameSelection> fs.


>+  ...When this returns TRUE, this computes aScrollableFrame
>+  // should be scrolled to where. 
Strange wording

This needs lots of practical testing, so this should be pushed to m-c right after branching to beta.
r+ if the questions about FindFirstNonNativeAnonymous() and generated content is answered.

Comment 71

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

Attachment: generated content testcase

Gecko and Webkit behave quite strangely with this kind of case.

Comment 72

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #70)
> Comment on attachment 543010 [details] [diff] [review] [review]
> Patch v5.4.1
> 
> 
> >+static nsIContent*
> >+GetContentToCaptureForSelection(nsIContent* aSelectionRoot)
> >+{
> >+  if (!aSelectionRoot->IsInNativeAnonymousSubtree()) {
> >+    return aSelectionRoot;
> >+  }
> >+  return aSelectionRoot->GetBindingParent();
> >+}
> Why this behavior?
> Why not returning aSelectionRoot->FindFirstNonNativeAnonymous()?

Oh, you're right. But I think that I shouldn't remove GetContentToCaptureForSelection() even if it just returns the result of FindFirstNonNativeAnonymous() because the static method have the logical meaning.

> Does this patch change selection handling when there is 
> generated content? (:before and :after)
> I'll upload a testcase.

I don't want to change it. But I should test it tomorrow.

> This needs lots of practical testing, so this should be pushed to m-c right
> after branching to beta.

Yeah, I agree with you.

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

> data:text/html,<style type="text/css">div{ width: 300px; height: 300px; overflow: auto; } p { margin: 0; padding: 0; white-space: pre; } p:before { content: "before before before before before "; } p:after { content: " after after after after after after after"; } </style><body><div id="inner"><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p><p>content content content</p></div></body>

I tested on this URL. If I mousedown on generated content, I cannot scroll by dragging on both current trunk and patched build. If I mousedown on content of <p>, I can scroll by dragging on both builds. So, I think the behavior isn't changed.

Comment 74

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v5.5 (mq)

Comment 75

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v5.5.1 (mq)

fix the typo in summary.

Comment 76

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/16e0e3787d46

added comments for static utility methods at landing.

Comment 77

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch v5.5.2 (mq)

Comment 78

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/16e0e3787d46

Comment 79

[Simon Howes]

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=nsFrame%3A%3AExpandSelectionByMouseMove&reason_type=contains&date=07%2F07%2F2011%2014%3A59%3A07&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=nsFrame%3A%3AExpandSelectionByMouseMove

Unsure if this patch is causing Nightly to crash when trying to make a comment on Facebook as an example.

Comment 80

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

If I read the stack correctly selectionRoot is null and GetPrimaryFrame() is
trying to access a member of selectionRoot, so the crash address is 0x48

Comment 81

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

..on 64bit. On 32bit the crash address is 0x28

Comment 82

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

Simon, do you know how to reproduce the crash? Exact steps would be great.

Comment 83

[Simon Howes]

Sure. Just click on a comment box on Facebook.

Comment 84

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

So far I haven't managed to reproduce.

Comment 85

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Temporarily, backed out for risk management of mozilla8, see bug 675865.

Comment 90

[Olmo]

Please see: http://jsfiddle.net/6333M/15/

I believe this bug is preventing the the mouseover from firing after you drag and hover over the "BLU" div.

Comment 91

[Olmo]

Please see: http://jsfiddle.net/6333M/15/

I believe this bug is preventing the the mouseover from firing after you drag and hover over the "BLU" div.

Comment 92

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Yes.

Comment 93

[crazymykl]

This is still happening as of FF 53.0a2.

Comment 94

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Resetting assignee which I don't work on in this several months.

Comment 95

[Negritas Sergiu, Desktop QA]

Hello Masayuki! Is still issue still valid and being worked on or this issue is not valid in the latest firefox versions and we can close it as WFM or other resolution.

I have tried to reproduce the issue with firefox 97.0a1(2021-12-07) on Windows 10 but for me looks fine for me.

Comment 96

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Indeed, it works. I'm not sure which bug has fixed this bug though. Thank you for letting me know, Negritas Sergiu!

(Edgar might know which bug fixed this so that ccing him.)