Closed Bug 786898 Opened 12 years ago Closed 12 years ago

If overflow-x or overflow-y causes no scroll only along the axis, the overflowDelta should be zero in some cases

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(2 files, 2 obsolete files)

This is a part of bug 674477.

If a scroll target's overflow-x or overflow-y is hidden but its ancestor is scrollable along the axis, probably users don't want to expect that horizontal swipe would cause moving back or forward in the history. In such case, we should set overflowDelta values 0 for preventing widget to perform additional action.
Summary: If overflow-x or overflow-y causes no scroll only along the axes, the overflowDelta should be zero in some cases → If overflow-x or overflow-y causes no scroll only along the axis, the overflowDelta should be zero in some cases
I think that if there is an ancestor which can scroll along the axis but current scroll target isn't scrollable along the axis due to overflow-*, ESM should consume the wheel event completely for preventing additional action such as history "Back" and "Forward" by horizontal swipe on Mac.

For doing that, if current scroll target cannot scroll along an axis due to overflow-*, check whether there is scrollable element in its ancestors. If there is it, set overflowDelta 0 for the axis. Otherwise, set delta value to overflowDelta.
Attachment #656786 - Attachment is obsolete: true
Attachment #657134 - Flags: review?(bugs)
I think that it doesn't make sense to return actual overflowDelta values because most callers don't use it. Instead of that, SendWheelEvent() should return error if overflow delta values are not expected. If the flags are not set, the check is never performed.
Attachment #657135 - Flags: superreview?(bugs)
Attachment #657135 - Flags: review?(bugs)
Comment on attachment 657135 [details] [diff] [review]
part.2 Add overflow delta value tests

Sorry for the spam. This replaces a necessary line with additional line.
Attachment #657135 - Flags: superreview?(bugs)
Attachment #657135 - Flags: review?(bugs)
Attachment #657135 - Flags: review-
Attachment #657148 - Flags: superreview?(bugs)
Attachment #657148 - Flags: review?(bugs)
Comment on attachment 657134 [details] [diff] [review]
part.1 Set overflowDelta 0 if the delta is unused by overflow-x: hidden; or overflow-y: hidden; but an ancestor is scrollable along the axis


> 
>+  bool beScrollableX =
>+    aEvent->deltaX && (aOptions & PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_X_AXIS);
>+  bool beScrollableY =
>+    aEvent->deltaY && (aOptions & PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_Y_AXIS);
>+
A bit odd variable names.


>   // If CSS overflow properties caused 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 = aEvent->deltaX;
>-  }
>-  if (overflowStyle.mVertical == NS_STYLE_OVERFLOW_HIDDEN) {
>-    aEvent->overflowDeltaY = aEvent->deltaY;
>+  // widget.  However, if there is another scrollable element in the ancestor
>+  // along the axis, probably users don't want the operation to cause
>+  // additional action such as moving history.  In such case, overflowDelta
>+  // values should be zero.
>+  if (scrollFrameWeak.IsAlive()) {
>+    if (aEvent->deltaX &&
>+        overflowStyle.mHorizontal == NS_STYLE_OVERFLOW_HIDDEN &&
>+        !ComputeScrollTarget(scrollFrame, aEvent,
>+                             COMPUTE_SCROLLABLE_ANCESTOR_ALONG_X_AXIS)) {
>+      aEvent->overflowDeltaX = aEvent->deltaX;
>+    }
>+    if (aEvent->deltaY &&
>+        overflowStyle.mVertical == NS_STYLE_OVERFLOW_HIDDEN &&
>+        !ComputeScrollTarget(scrollFrame, aEvent,
>+                             COMPUTE_SCROLLABLE_ANCESTOR_ALONG_Y_AXIS)) {
>+      aEvent->overflowDeltaY = aEvent->deltaY;
>+    }
>   }
This is not clear. You talk about setting overflowDelta to 0, but set it actually to
aEvent->delta. Perhaps you should say
"In such case overflowDelta should stay zero".


>+  enum ComputeScrollTargetOptions
>+  {
>+    PREFER_MOUSE_WHEEL_TRANSACTION               = 1,
>+    PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_X_AXIS = 2,
>+    PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_Y_AXIS = 4,
>+    START_FROM_PARENT                            = 8,
>+
>+    COMPUTE_LEGACY_MOUSE_SCROLL_EVENT_TARGET     = 0,
Please make this the first item, or change it to use some other value.
Attachment #657134 - Flags: review?(bugs) → review+
Comment on attachment 657148 [details] [diff] [review]
part.2 Add overflow delta value tests

>+   // If following flags are specified, this method will fail if the
>+   // overflowDelta values are unexpected value.
Maybe something like:
If any of the following flags is specified this method will throw an exception
in case the relevant overflowDelta has an unexpected value.
Attachment #657148 - Flags: superreview?(bugs)
Attachment #657148 - Flags: superreview+
Attachment #657148 - Flags: review?(bugs)
Attachment #657148 - Flags: review+
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.