element with overflow-x:hidden can be scrolled horizontal

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: atiware, Assigned: masayuki)

Tracking

Trunk
mozilla18
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 548719 [details]
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
(Reporter)

Updated

7 years ago
Attachment #548719 - Attachment mime type: text/plain → text/html

Comment 1

7 years ago
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
(Reporter)

Comment 2

7 years ago
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."
(Reporter)

Updated

7 years ago
Component: General → General
Product: Firefox → Core
Version: unspecified → Trunk

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: general → general

Updated

6 years ago
Duplicate of this bug: 783744
Component: General → Event Handling
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Created attachment 653158 [details] [diff] [review]
Patch

This patch also fixes setting negative value to overflowDelta*.
Attachment #653158 - Flags: review?(bugs)
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?
> 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 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.
Attachment #653158 - Flags: review?(bugs) → review-
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?
Created attachment 653261 [details] [diff] [review]
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.
Attachment #653158 - Attachment is obsolete: true
Attachment #653261 - Flags: review?(jmathies)
Attachment #653261 - Flags: review?(bugs)
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.
Attachment #653261 - Flags: review?(jmathies)
Attachment #653261 - Flags: review?(bugs)
Created attachment 655968 [details] [diff] [review]
Patch

We should just set unused delta values to overflowDelta* when the default action is scroll.
Attachment #653261 - Attachment is obsolete: true
Attachment #655968 - Flags: review?(bugs)
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?
Attachment #655968 - Flags: review?(bugs) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/3eb0de1d8c3b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.