Closed
Bug 259615
Opened 19 years ago
Closed 19 years ago
'overflow: hidden' elements shouldn't be scrollable by the user
Categories
(Core :: DOM: Selection, defect)
Core
DOM: Selection
Tracking
()
RESOLVED
FIXED
mozilla1.8alpha4
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Whiteboard: [patch])
Attachments
(4 files, 2 obsolete files)
1.37 KB,
text/html; charset=UTF-8
|
Details | |
1.42 KB,
text/html; charset=UTF-8
|
Details | |
23.75 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•19 years ago
|
||
Assignee | ||
Comment 2•19 years ago
|
||
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).
sounds great
Assignee | ||
Comment 4•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Attachment #159002 -
Attachment description: testcase → testcase for drag(selection)-scrolling
Assignee | ||
Comment 5•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Summary: 'overflow: hidden' elements shouldn't be drag-selectable by the user → 'overflow: hidden' elements shouldn't be scrollable by the user
Assignee | ||
Updated•19 years ago
|
Attachment #159046 -
Flags: superreview?(roc)
Attachment #159046 -
Flags: review?(roc)
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8alpha4
Assignee | ||
Comment 6•19 years ago
|
||
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?
Assignee | ||
Comment 8•19 years ago
|
||
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.
Assignee | ||
Comment 9•19 years ago
|
||
*** 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?
Assignee | ||
Comment 11•19 years ago
|
||
Attachment #159046 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #159046 -
Flags: superreview?(roc)
Attachment #159046 -
Flags: review?(roc)
Assignee | ||
Updated•19 years ago
|
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+
Assignee | ||
Comment 13•19 years ago
|
||
No, it can't, since there's an else, and we don't want to do either half.
Assignee | ||
Comment 14•19 years ago
|
||
Attachment #159507 -
Attachment is obsolete: true
Attachment #159517 -
Flags: superreview+
Attachment #159517 -
Flags: review+
Assignee | ||
Comment 15•19 years ago
|
||
Fix checked in to trunk, 2004-09-20 21:41 -0700.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 16•19 years ago
|
||
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.
Assignee | ||
Comment 18•19 years ago
|
||
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 → ---
Assignee | ||
Comment 19•19 years ago
|
||
My previous comment doesn't entirely make sense. But anyway, this should do it, although I can't test wheel scrolling on my machine.
Assignee | ||
Updated•19 years ago
|
Attachment #161001 -
Flags: superreview?(roc)
Attachment #161001 -
Flags: review?(roc)
Assignee | ||
Comment 20•19 years ago
|
||
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•19 years ago
|
||
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+
Assignee | ||
Comment 22•19 years ago
|
||
Remaining fix (attachment 161001 [details] [diff] [review]) checked in to trunk, 2004-10-08 22:15 -0700.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Comment 23•19 years ago
|
||
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?
Updated•19 years ago
|
Flags: blocking-aviary1.0? → blocking-aviary1.0-
Comment 24•19 years ago
|
||
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.
Assignee | ||
Comment 25•19 years ago
|
||
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•19 years ago
|
||
*** Bug 270362 has been marked as a duplicate of this bug. ***
Comment 27•18 years ago
|
||
*** Bug 277952 has been marked as a duplicate of this bug. ***
Comment 28•17 years ago
|
||
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•17 years ago
|
||
Filed bug 325942 for comment 28
Comment 30•17 years ago
|
||
*** Bug 254364 has been marked as a duplicate of this bug. ***
Comment 31•17 years ago
|
||
*** 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.
Description
•