Closed
Bug 143815
Opened 23 years ago
Closed 23 years ago
[FIXr]clicking link on page with body{overflow:auto} scrolls to top
Categories
(Core :: Layout, defect, P1)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.3alpha
People
(Reporter: jruderman, Assigned: bzbarsky)
References
()
Details
(Keywords: access, regression, testcase)
Attachments
(5 files, 1 obsolete file)
|
587 bytes,
text/html
|
Details | |
|
602 bytes,
text/html
|
Details | |
|
601 bytes,
text/html
|
Details | |
|
916 bytes,
text/html
|
Details | |
|
17.28 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
http://tantek.com/favelets/ uses "body { overflow:auto }". When I click a link
in the page, Mozilla scrolls to the top of the page and doesn't follow the link
as a result.
I have no idea why Tantek uses body { overflow:auto } for his page. It breaks
panning in Internet Explorer and causes two scrollbars to appear when I increase
the font size in Mozilla, but it doesn't affect the appearance of the page.
|
Reporter | |
Comment 1•23 years ago
|
||
|
Assignee | |
Comment 2•23 years ago
|
||
Even right-clicking does this. It appears that ScrollFrameIntoView is broken
here somehow...
Also, this completely breaks tabbing to the lower anchors on the page -- we
never scroll to them.
|
|
Assignee | |
Comment 3•23 years ago
|
||
Did some investigating. Trunk linux builds, this works correctly in the
2001-11-17-21 build and is broken in the 2001-11-18-06 build. I suspect the
checkin for bug 65236 is responsible (that regressed some other situations of
this sort as well).
|
|
Assignee | |
Comment 5•23 years ago
|
||
So.. the problem is that PresShell::ScrollFrameIntoView now seems to assume
that no frame is taller than screen-height... In this case the <body> is
scrollable. We scroll it so that the link is visible in the <body>. Then we
scroll the viewport so that the <body> is visible in the viewport, but that
just scrolls to top since the <body> is already visible.
This is very obvious in this testcase if you window height is below 600px or so
-- the body has a height of 800px, so the scrollbars should show up... if they
do not, zoom the font up a bit.
|
|
||
Comment 7•23 years ago
|
||
adding the testcase keyword and setting the priority to P1 since this is a
regression.
|
|
||
Comment 8•23 years ago
|
||
nsbeta1-. body { overflow:auto } is not commonly used.
|
|
Assignee | |
Comment 9•23 years ago
|
||
I'd just like to point out that this will be a problem with any overflow:auto or
overflow:scroll element that is taller than the viewport. This is by no means
limited to <body> -- the impact is much more widespread. Renominating, and my
apologies to the ADT for not being clearer the first time around.
|
|
Assignee | |
Comment 10•23 years ago
|
||
tabbing to the third link does not work, nor does clicking on it. It cannot be
followed.
|
|
Reporter | |
Comment 11•23 years ago
|
||
> Tabbing to the third link does not work, nor does clicking on it. It cannot be
followed.
You can follow the link by pressing enter after clicking on it, but very few
users would figure that out. To most mouse users and most keyboard users, the
link seems to be impossible to follow.
|
|
||
Updated•23 years ago
|
Assignee: attinasi → alexsavulov
|
|
||
Comment 12•23 years ago
|
||
Reassigning to Alex.
|
|
||
Comment 13•23 years ago
|
||
Reassigning to jst since this bug seems to be caused by the fix for bug 62536
Assignee: alexsavulov → jst
|
|
Assignee | |
Comment 14•23 years ago
|
||
*** Bug 146770 has been marked as a duplicate of this bug. ***
|
|
Assignee | |
Comment 15•23 years ago
|
||
*** Bug 178148 has been marked as a duplicate of this bug. ***
|
|
Assignee | |
Comment 16•23 years ago
|
||
taking; I have a working fix that I just need to clean up.
Assignee: jst → bzbarsky
Target Milestone: --- → mozilla1.3alpha
|
|
Assignee | |
Comment 17•23 years ago
|
||
|
|
Assignee | |
Updated•23 years ago
|
Summary: clicking link on page with body{overflow:auto} scrolls to top → [FIX]clicking link on page with body{overflow:auto} scrolls to top
+ if (aRect.YMost() < visibleRect.y) {
...
+ } else if (aRect.y > visibleRect.YMost()) {
...
+ if (aRect.XMost() < visibleRect.x) {
...
+ } else if (aRect.x > visibleRect.XMost()) {
I know you're just copying and pasting code, but these should be fixed to
include a "fuzz factor" so that a 1-twip overlap doesn't count as "visible".
Maybe you could call aScrollingView->GetLineHeight to get an appropriate epsilon?
+ nscoord frameAlignY = aRect.y + (aRect.height * aVPercent) / 100;
+ scrollOffsetY = frameAlignY - (visibleRect.height * aVPercent) / 100;
...
+ nscoord frameAlignX = aRect.x + (aRect.width * aHPercent) / 100;
+ scrollOffsetX = frameAlignX - (visibleRect.width * aHPercent) / 100;
Again you're just copying code, but the multiplication could overflow for a
frame that's only about a million pixels high. How about doing the arithmetic
using double?
Other than that, looks great.
|
||
Comment 19•23 years ago
|
||
Comment on attachment 105074 [details] [diff] [review]
fix
- In PresShell::ScrollFrameIntoView()
+ nsIScrollableView* scrollingView = nsnull;
+
+#ifdef DEBUG
+ if (closestView) {
+ CallQueryInterface(closestView, &scrollingView);
+ NS_ASSERTION(!scrollingView,
+ "What happened to the scrolled view? "
+ "The frame should not be directly in the scrolling view!");
+ }
+#endif
...
Don't set variables defined outside #ifdef DEBUG in code that's inside #ifdef
DEBUG, the debug code could affect how the code outside the #ifdef DEBUG works
and make optimized code not work the same way debug code does. Declare a new
nsIScrollableView inside the #ifdef DEBUG block and use that in the debug-only
code.
sr=jst with that change.
Attachment #105074 -
Flags: superreview+
|
|
Assignee | |
Comment 20•23 years ago
|
||
|
|
Assignee | |
Comment 21•23 years ago
|
||
The only users of SCROLL_IF_NOT_VISIBLE are form control frames (that's what
they do when focus() is called on the control). The only one of these for which
that _may_ make sense (instead of just doing SCROLL_ANYWHERE) is <textarea> --
with a large textarea the end of which is visible, focus() shouldn't scroll to
the top of the textarea,,,,
|
|
Assignee | |
Comment 22•23 years ago
|
||
I'll talk to jkeiser about the form control's bizarre use of
SCROLL_IF_NOT_VISIBLE...
Attachment #105074 -
Attachment is obsolete: true
|
||
Comment 23•23 years ago
|
||
*** Bug 115505 has been marked as a duplicate of this bug. ***
Comment on attachment 105566 [details] [diff] [review]
fix the review comments
sr=roc+moz
Attachment #105566 -
Flags: superreview+
|
|
Assignee | |
Updated•23 years ago
|
Summary: [FIX]clicking link on page with body{overflow:auto} scrolls to top → [FIXr]clicking link on page with body{overflow:auto} scrolls to top
|
|
Assignee | |
Comment 25•23 years ago
|
||
checked in, r=roc+moz (permission to switch the sr to r via email), sr=jst
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Comment 26•23 years ago
|
||
*** Bug 179883 has been marked as a duplicate of this bug. ***
|
|
Assignee | |
Comment 27•23 years ago
|
||
*** Bug 182199 has been marked as a duplicate of this bug. ***
|
|
Assignee | |
Comment 28•22 years ago
|
||
*** Bug 184473 has been marked as a duplicate of this bug. ***
|
|
Assignee | |
Comment 29•22 years ago
|
||
*** Bug 188863 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
•