Closed Bug 259615 Opened 16 years ago Closed 16 years ago

'overflow: hidden' elements shouldn't be scrollable by the user

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8alpha4

People

(Reporter: dbaron, Assigned: dbaron)

References

(Depends on 1 open bug)

Details

(Whiteboard: [patch])

Attachments

(4 files, 2 obsolete files)

testcase for drag(selection)-scrolling
1.37 KB, text/html; charset=UTF-8
Details
testcase for keybord scrolling (when checkbox is focused)
1.42 KB, text/html; charset=UTF-8
Details
patch
23.75 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review
patch for wheel mouse
2.48 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review 
A side-effect of making 'overflow: hidden' create a scroll frame so that it 
could be scrollable programmatically by the page was that it also allowed
'overflow: hidden' elements to be scrollable by drag-selection.  This is not
what IE does and it's probably a bad thing since it allows the user to get into
an odd state that it isn't obvious how to get out of (since there are no
scrollbars).

Steps to reproduce:
 1. load testcase
 2. select the upper chunk of text with the mouse and then move the mouse down

Expected results:
 1. text doesn't move

Actual results:
 2. text moves when you hit the bottom of the div
I think all the users of GetNearestScrollingView are finding something for the
user to scroll.  So I think the solution here is to make GetNearestScrollingView
take a parameter indicating whether it's for horizontal or vertical scrolling
and then check the overflow values on the scrollframe (using nsIScrollableFrame,
not the style data).
Attached patch patch (obsolete) — — Splinter Review
Attachment #159002 - Attachment description: testcase → testcase for drag(selection)-scrolling
Summary: 'overflow: hidden' elements shouldn't be drag-selectable by the user → 'overflow: hidden' elements shouldn't be scrollable by the user
Attachment #159046 - Flags: superreview?(roc)
Attachment #159046 - Flags: review?(roc)
Status: NEW → ASSIGNED
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8alpha4
This prevents user scrolling in the first half of both testcases and attachment
25609 [details] still works.
Seems to me that a slightly more logical approach would be for the direction to
have three possible values: horizontal, vertical, or either. The comment should
emphasize that GetNearestScrollingView returns the nearest view that is *user
scrollable* in the direction(s) indicated. And in fact I think in most
situations we should just default to 'either', which would simplify your patch.
The only time we care about the direction is for user keys, I think.

I'm a bit afraid that your loop is going to do weird things, for example if I
have element A which can scroll vertically containing element B which can scroll
horizontally, I really don't want a user action to scroll both B and A. Do I?
It's worth noting that the |aScrollParentViews| parameter is always true --
probably I should remove it, since this change does make it make less sense for
it to be false.  So right now, what we're doing is scrolling everything that's
scrollable.  I could make it so it gets anything that's scrollable either way,
but then I'd need to add the scrollability checking in a whole bunch of places
in the nsSelection.cpp code -- I think that seems more complicated than what I'm
doing now.
*** Bug 220718 has been marked as a duplicate of this bug. ***
I still don't like the direction loop. It seems wrong to issue two scroll
operations if we have to scroll in both directions.

> I'd need to add the scrollability checking in a whole bunch of places in the
> nsSelection.cpp code

Can you give an example?
Attached patch patch (obsolete) — — Splinter Review
Attachment #159046 - Attachment is obsolete: true
Attachment #159046 - Flags: superreview?(roc)
Attachment #159046 - Flags: review?(roc)
Attachment #159507 - Flags: superreview?(roc)
Attachment #159507 - Flags: review?(roc)
Comment on attachment 159507 [details] [diff] [review]
patch

How about moving ViewToScrollbarStyles to nsLayoutUtils and call it from
GetNearestScrollingView?

+  if (ss.mVertical != NS_STYLE_OVERFLOW_HIDDEN) {
   if (NS_PRESSHELL_SCROLL_ANYWHERE == aVPercent) {

could just be &&

+  if (ss.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN) {
   if (NS_PRESSHELL_SCROLL_ANYWHERE == aHPercent) {

ditto
Attachment #159507 - Flags: superreview?(roc)
Attachment #159507 - Flags: superreview+
Attachment #159507 - Flags: review?(roc)
Attachment #159507 - Flags: review+
No, it can't, since there's an else, and we don't want to do either half.
Attached patch patch — — Splinter Review
Attachment #159507 - Attachment is obsolete: true
Fix checked in to trunk, 2004-09-20 21:41 -0700.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I can still scroll those 'overflow:hidden' elements with my mousewheel.
Maybe that should also not be allowed? (IE doesn't allow it)
Yeah, that should not be allowed.
Reopening.  There are also probably some other edge cases related to
nsIScrollableViewProvider in PresShell::GetViewToScroll.  I'm not sure exactly
why we want nsIScrollableViewProvider.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
My previous comment doesn't entirely make sense.  But anyway, this should do
it, although I can't test wheel scrolling on my machine.
Attachment #161001 - Flags: superreview?(roc)
Attachment #161001 - Flags: review?(roc)
But actually, comment 18 is correct, except that the last sentence should
probably be complaining about DoScrollText overusing it, not whether it should
exist at all.
I've tried the patch for wheel mouse in my debug build.
With that, I can't scroll those 'overflow:hidden' elements with my mousewheel
anymore.
Attachment #161001 - Flags: superreview?(roc)
Attachment #161001 - Flags: superreview+
Attachment #161001 - Flags: review?(roc)
Attachment #161001 - Flags: review+
Remaining fix (attachment 161001 [details] [diff] [review]) checked in to trunk, 2004-10-08 22:15 -0700.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Please incorporate this patch in to 1.0; our company's web-based product relys
heavily on IFRAME's and this has caused us to have to recommend to our customers
to never use their mouse wheel while on our software.
Flags: blocking-aviary1.0?
Flags: blocking-aviary1.0? → blocking-aviary1.0-
The commentless Asa denial has struck again. Well, since I have no idea /why/ I
was denied, I guess I can't appeal or the discuss the technical merits of this
being in 1.0. Damn you, Asa.
It should be obvious that this isn't appropriate for 1.0.  It depends on a whole
bunch of large patches that are too risky for a release that close.
Blocks: 194904
*** Bug 270362 has been marked as a duplicate of this bug. ***
*** Bug 277952 has been marked as a duplicate of this bug. ***
This still occurs when a hash is used in a URL and overflow is hidden.

e.g. http://www.leevigraham.com/holy_grail_bug_example.html
and http://www.leevigraham.com/holy_grail_bug_example.html#middle
Depends on: 325942
*** Bug 254364 has been marked as a duplicate of this bug. ***
*** Bug 325731 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.