674477 - element with overflow-x:hidden can be scrolled horizontal

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Attila Soki]

Attachment: testcase 1

a div element with fixed dimensions and overflow-x:hidden can be scrolled horizontal.

how to-repeat:
1. open testcase 1
2. click or mouse over the box
3. with your mouse or trackpad scroll the content of the box vertical
4. now you can scroll horizontal with trackpad sweep or trackball

Comment 1

[Vlad [QA]]

I can scroll horizontally. Is this intended? 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a1) Gecko/20110728 Firefox/8.0a1

Comment 2

[Attila Soki]

you should not be able to scroll

see:
https://developer.mozilla.org/En/CSS/Overflow-x
http://www.w3.org/TR/css3-box/#overflow

"hidden
    This value indicates that the content is clipped and that no scrolling mechanism should be provided to view the content outside the clipping region."

Comment 4

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

Attachment: Patch

This patch also fixes setting negative value to overflowDelta*.

Comment 5

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

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=2645700f5cf3

Comment 6

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

Comment on attachment 653158 [details] [diff] [review]
Patch

># HG changeset patch
># Parent 9c48df21d74453daafabd9388e14c5b7d442a747
>Bug 674477 Don't scroll around axis whose overflow is hidden by wheel events
>
>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
>--- a/content/events/src/nsEventStateManager.cpp
>+++ b/content/events/src/nsEventStateManager.cpp
>@@ -2835,16 +2835,26 @@ nsEventStateManager::DoScrollText(nsIScr
>   nsSize scrollAmount = GetScrollAmount(pc, aEvent, aScrollableFrame);
>   nsIntSize scrollAmountInDevPixels(
>     pc->AppUnitsToDevPixels(scrollAmount.width),
>     pc->AppUnitsToDevPixels(scrollAmount.height));
>   nsIntPoint actualDevPixelScrollAmount =
>     DeltaAccumulator::GetInstance()->
>       ComputeScrollAmountForDefaultAction(aEvent, scrollAmountInDevPixels);
> 
>+  // Don't scroll around the axis whose overflow style is hidden.
>+  nsPresContext::ScrollbarStyles overflowStyle =
>+                                   aScrollableFrame->GetScrollbarStyles();
>+  if (overflowStyle.mHorizontal == NS_STYLE_OVERFLOW_HIDDEN) {
>+    actualDevPixelScrollAmount.x = 0;
>+  }
>+  if (overflowStyle.mVertical == NS_STYLE_OVERFLOW_HIDDEN) {
>+    actualDevPixelScrollAmount.y = 0;
>+  }
>+
>   nsIAtom* origin = nullptr;
>   switch (aEvent->deltaMode) {
>     case nsIDOMWheelEvent::DOM_DELTA_LINE:
>       origin = nsGkAtoms::mouseWheel;
>       break;
>     case nsIDOMWheelEvent::DOM_DELTA_PAGE:
>       origin = nsGkAtoms::pages;
>       break;
>@@ -2907,16 +2917,26 @@ nsEventStateManager::DoScrollText(nsIScr
>     aEvent->overflowDeltaX = overflow.x;
>     aEvent->overflowDeltaY = overflow.y;
>   } else {
>     aEvent->overflowDeltaX =
>       static_cast<double>(overflow.x) / scrollAmountInDevPixels.width;
>     aEvent->overflowDeltaY =
>       static_cast<double>(overflow.y) / scrollAmountInDevPixels.height;
>   }
>+
>+  // If overflow causes not to scroll, the overflowDelta* values should be
>+  // same as delta* values since they may be used as gesture event by widget.
>+  if (overflowStyle.mHorizontal == NS_STYLE_OVERFLOW_HIDDEN) {
>+    aEvent->overflowDeltaX = NS_ABS(aEvent->deltaX);
>+  }
>+  if (overflowStyle.mVertical == NS_STYLE_OVERFLOW_HIDDEN) {
>+    aEvent->overflowDeltaY = NS_ABS(aEvent->deltaY);
>+  }
>+
How did we lose this? Where was this implemented earlier?
Why isn't the check in nsEventStateManager::ComputeScrollTarget enough?

>             ComputeScrollTarget(aTargetFrame, wheelEvent, true);
>-          wheelEvent->overflowDeltaX = wheelEvent->deltaX;
>-          wheelEvent->overflowDeltaY = wheelEvent->deltaY;
>+          wheelEvent->overflowDeltaX = NS_ABS(wheelEvent->deltaX);
>+          wheelEvent->overflowDeltaY = NS_ABS(wheelEvent->deltaY);
...
>   if (mMultiplierX[index]) {
>-    aEvent->overflowDeltaX /= mMultiplierX[index];
>+    aEvent->overflowDeltaX /= NS_ABS(mMultiplierX[index]);
>   }
>   if (mMultiplierY[index]) {
>-    aEvent->overflowDeltaY /= mMultiplierY[index];
>+    aEvent->overflowDeltaY /= NS_ABS(mMultiplierY[index]);
>   }
> }

Now I'm lost... Why overflowDeltaX/Y needs to be positive always?
If so, it should be unsigned double.
(And the comment about those member variables of WheelEvent should be improved.)
But still odd... Should the whoever uses the value use NS_ABS is needed?

Comment 7

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

er, if needed.

Comment 8

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

> How did we lose this? Where was this implemented earlier?
> Why isn't the check in nsEventStateManager::ComputeScrollTarget enough?

ComputeScrollTarget() skips overflow hidden elements if the wheel event cannot scroll around either axis. For example, if an event is oblique scroll event like: { deltaX: 1.0, deltaY: 1.0 }, ComputeScrollTarget() returns an element which is scrollable to bottom *or* right. So, even if the element is overflow-x: hidden; or overflow-y: hidden, it *can* be scrolled to bottom or right. So, we need to cancel the scroll at DoScrollText() for overflow-x: hidden; or overflow-y: hidden (not overflow: hidden).

I'm not sure which bug is a regression of this. This is not a regression of the D3E WheelEvent related changes.

> >             ComputeScrollTarget(aTargetFrame, wheelEvent, true);
> >-          wheelEvent->overflowDeltaX = wheelEvent->deltaX;
> >-          wheelEvent->overflowDeltaY = wheelEvent->deltaY;
> >+          wheelEvent->overflowDeltaX = NS_ABS(wheelEvent->deltaX);
> >+          wheelEvent->overflowDeltaY = NS_ABS(wheelEvent->deltaY);
> ...
> >   if (mMultiplierX[index]) {
> >-    aEvent->overflowDeltaX /= mMultiplierX[index];
> >+    aEvent->overflowDeltaX /= NS_ABS(mMultiplierX[index]);
> >   }
> >   if (mMultiplierY[index]) {
> >-    aEvent->overflowDeltaY /= mMultiplierY[index];
> >+    aEvent->overflowDeltaY /= NS_ABS(mMultiplierY[index]);
> >   }
> > }
> 
> Now I'm lost... Why overflowDeltaX/Y needs to be positive always?

Loos like so. See nsGfxScrollFrame.cpp:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2535
> 2535   if (aOverflow) {
> 2536     nsPoint clampAmount = mDestination - newPos;
> 2537     float appUnitsPerDevPixel = mOuter->PresContext()->AppUnitsPerDevPixel();
> 2538     *aOverflow = nsIntPoint(
> 2539         NSAppUnitsToIntPixels(NS_ABS(clampAmount.x), appUnitsPerDevPixel),
> 2540         NSAppUnitsToIntPixels(NS_ABS(clampAmount.y), appUnitsPerDevPixel));
> 2541   }

I don't know the reason, but this is the usual case to return overflowDelta* to widget.

I realized that the old nsEventStateManager::DoScrollText() had set signed delta values if there is no scrollable target. See #2885 of this patch:
https://bugzilla.mozilla.org/attachment.cgi?id=650055&action=diff

I guess that setting signed value to nsMouseScrollEvent::scrollOverflow was a bug, and nsGfxScrollFrame::ScrollBy() does it intentionally. Though, I don't know the reason.

Now, nsIDOMWindowUtils::sendWheelEvent() this is checking:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#650

I realized this issue by the new tests with this check.

> If so, it should be unsigned double.
> (And the comment about those member variables of WheelEvent should be
> improved.)

The nsMouseScrollEvent::scrollOverflow was signed int:
https://bugzilla.mozilla.org/attachment.cgi?id=650077&action=diff

But sounds OK for me.

> But still odd... Should the whoever uses the value use NS_ABS is needed?

There are two users of overflowDelta*.

One is [ChildView maybeTrackScrollEventAsSwipe: scrollOverflow:] http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#3060
It's only checks if overflowDeltaX is zero or non-zero. If it's non-zero, it might cause swipe gesture action such as "back" or "forward".

The other is nsWindow::OnGesture() of Windows. It calls nsWinGesture::UpdatePanFeedbackX() and nsWinGesture::UpdatePanFeedbackY():
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWinGesture.cpp#483
Looks like they expect "signed" overflow value...

Comment 9

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

Comment on attachment 653158 [details] [diff] [review]
Patch

So, I don't like this this way.
Either we report also negative value to caller, or the type
should be unsigned.

Comment 10

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

Looks like nsEventStateManager::DoScrollText() is only one user of nsGfxScrollFrame::ScrollBy()'s overflow argument. Should we change nsGfxScrollFrame side?

But looks like the original overflow computation needs unsigned overflow value:
http://hg.mozilla.org/mozilla-central/annotate/274480cf21d7/view/src/nsScrollPortView.cpp#l376

This is implemented by jimm. So, I may misread the nsWinGesture code.

Jimm: widget expects unsigned overflow values for wheel events? Why?

Comment 11

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

Attachment: Patch

Okay, then, I think that there is no reason to lose the sign. If Windows need unsigned overflowDelta, widget should use NS_ABS() itself.

Comment 12

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

Comment on attachment 653261 [details] [diff] [review]
Patch

Oops, the sign of aOverflow of nsGfxScrollFrame::ScrollBy() may be different from ESM. I'll check it tomorrow.

Comment 13

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

Attachment: Patch

We should just set unused delta values to overflowDelta* when the default action is scroll.

Comment 14

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

Comment on attachment 655968 [details] [diff] [review]
Patch

But please file bugs about the XXX issues.
Especially the first on about back/forward looks worrisome.
Can a page which has overflow: hidden in the root disable
non-scrolling special cases?

//     However, these values are applied the pref values.
I don't understand what this means. Does "these" refer to
delta or overflowDelta values?

Comment 15

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

(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 655968 [details] [diff] [review]
> Patch
> 
> But please file bugs about the XXX issues.
> Especially the first on about back/forward looks worrisome.
> Can a page which has overflow: hidden in the root disable
> non-scrolling special cases?

Ah, no. I meant that with non-zero overflowDeltaX value, nsChildView (widget/cocoa) may dispatch horizontal swipe gesture event and it will cause "back" or "forward" in the history.

If a user scrolled vertically on a scrollable element whose overflow-x is hidden, later horizontal swipe operation can cause the "back" or "forward" if it's performed in a short time.

So, I think that if the scroll target is decided from wheel transaction, the event's overflowDelta should be 0 if user cannot scroll around the axis due to overflow-x: hidden; or overflow-y: hidden; because we're *assuming* that the user really wants to scroll the element by the operation during a wheel transaction.

So, the expected bug is, horizontal swipe gesture may cause moving "back" or "forward" in the history unexpectedly on a overflow-x: hidden element.

> //     However, these values are applied the pref values.
> I don't understand what this means. Does "these" refer to
> delta or overflowDelta values?

Ah, yes, but I meant overflowDelta values are NOT applied the pref values. It's just typo.

After dispatching a wheel event from widget, deltaX/deltaY/deltaZ are applied pref values. But the overflowDelta would be unused "original" delta values (i.e., delta_multiplier pref values are canceled by DeltaAccumulator::CancelApplyingUserPrefsFromOverflowDelta()) when the default action is scroll.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb0de1d8c3b

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3eb0de1d8c3b