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)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
Attachments
(2 files, 2 obsolete files)
13.22 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
67.16 KB,
patch
|
smaug
:
review+
smaug
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
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
Assignee | ||
Comment 1•12 years ago
|
||
I'm writing additional tests for overflowDelta values...
Assignee | ||
Comment 2•12 years ago
|
||
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)
Assignee | ||
Comment 3•12 years ago
|
||
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)
Assignee | ||
Comment 4•12 years ago
|
||
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-
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #657135 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #657148 -
Flags: superreview?(bugs)
Attachment #657148 -
Flags: review?(bugs)
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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+
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/083d6ef2f044 https://hg.mozilla.org/integration/mozilla-inbound/rev/28da731d9959
Target Milestone: --- → mozilla18
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/083d6ef2f044 https://hg.mozilla.org/mozilla-central/rev/28da731d9959
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•