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

Status: RESOLVED FIXED

(Core :: Selection, defect)

Description

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

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

Comment 1

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Attachment: testcase for drag(selection)-scrolling

Comment 2

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

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).

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

sounds great

Comment 4

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Attachment: patch

Comment 5

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Attachment: testcase for keybord scrolling (when checkbox is focused)

Comment 6

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

This prevents user scrolling in the first half of both testcases and attachment
25609 [details] still works.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 8

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

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.

Comment 9

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

*** Bug 220718 has been marked as a duplicate of this bug. ***

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 11

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Attachment: patch

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 13

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

No, it can't, since there's an else, and we don't want to do either half.

Comment 14

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Attachment: patch

Comment 15

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Fix checked in to trunk, 2004-09-20 21:41 -0700.

Comment 16

[Martijn Wargers (zombie)]

I can still scroll those 'overflow:hidden' elements with my mousewheel.
Maybe that should also not be allowed? (IE doesn't allow it)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, that should not be allowed.

Comment 18

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Reopening.  There are also probably some other edge cases related to
nsIScrollableViewProvider in PresShell::GetViewToScroll.  I'm not sure exactly
why we want nsIScrollableViewProvider.

Comment 19

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Attachment: patch for wheel mouse

My previous comment doesn't entirely make sense.  But anyway, this should do
it, although I can't test wheel scrolling on my machine.

Comment 20

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

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.

Comment 21

[Martijn Wargers (zombie)]

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.

Comment 22

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Remaining fix (attachment 161001 [details] [diff] [review]) checked in to trunk, 2004-10-08 22:15 -0700.

Comment 23

[Jason Clinton]

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.

Comment 24

[Jason Clinton]

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.

Comment 25

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

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.

Comment 26

[Robert]

*** Bug 270362 has been marked as a duplicate of this bug. ***

Comment 27

[PikeUK]

*** Bug 277952 has been marked as a duplicate of this bug. ***

Comment 28

[travis]

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

Comment 29

[Mats Palmgren (:mats)]

Filed bug 325942 for comment 28

Comment 30

[Jesse Ruderman]

*** Bug 254364 has been marked as a duplicate of this bug. ***

Comment 31

[Martijn Wargers (zombie)]

*** Bug 325731 has been marked as a duplicate of this bug. ***