Closed Bug 674477 Opened 9 years ago Closed 8 years ago

element with overflow-x:hidden can be scrolled horizontal

Categories

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

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: atiware, Assigned: masayuki)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file 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
Attachment #548719 - Attachment mime type: text/plain → text/html
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
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."
Product: Firefox → Core
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: general → general
Duplicate of this bug: 783744
Component: General → Event Handling
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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?
er, if 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?
Attached patch Patch (obsolete) — Splinter Review
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)
Attached patch PatchSplinter Review
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
Closed: 8 years ago
Resolution: --- → FIXED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.