Last Comment Bug 105894 - Clicking a partially off-screen link shouldn't scroll the page
: Clicking a partially off-screen link shouldn't scroll the page
Status: VERIFIED FIXED
Image map's issue(bug 288945) is seri...
: verified1.8.0.1, verified1.8.1
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: All All
: P3 minor with 1 vote (vote)
: mozilla1.8.1
Assigned To: Masayuki Nakano [:masayuki]
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
: 146484 308892 308919 316603 (view as bug list)
Depends on: 376785
Blocks: 288945 300765
  Show dependency treegraph
 
Reported: 2001-10-21 04:44 PDT by Matthew Paul Thomas
Modified: 2014-10-11 16:51 PDT (History)
25 users (show)
dveditz: blocking1.8.0.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch rv1.0 (2.55 KB, patch)
2005-09-17 09:55 PDT, Masayuki Nakano [:masayuki]
roc: review-
Details | Diff | Splinter Review
Patch 1.1 (2.25 KB, patch)
2005-09-17 21:38 PDT, Masayuki Nakano [:masayuki]
roc: review+
roc: superreview+
Details | Diff | Splinter Review
Patch for check-in (2.40 KB, patch)
2005-09-17 21:53 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch for check-in (2.40 KB, patch)
2005-09-17 21:54 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
Patch for check-in (2.41 KB, patch)
2005-09-18 04:30 PDT, Masayuki Nakano [:masayuki]
no flags Details | Diff | Splinter Review
patch for 1.8 branch (5.55 KB, patch)
2005-12-01 17:13 PST, Michael Newton
masayuki: review+
masayuki: superreview+
dveditz: approval1.8.0.1+
dveditz: approval1.8.1+
Details | Diff | Splinter Review

Description User image Matthew Paul Thomas 2001-10-21 04:44:35 PDT
Build: 2001101904, Mac OS 9.1

To reproduce:
1.  Go to any page which is long and has links on it.
2.  Scroll so that a link is partly off-screen.
3.  Click on the link.

What should happen:
*   A focus ring is drawn around the link.
*   The linked URI is opened.

What actually happens:
*   The page scrolls so that all of the link is visible (if possible).
*   A focus ring is drawn around the link.
*   The linked URI is opened.
Comment 1 User image Matthew Paul Thomas 2001-10-21 04:45:37 PDT
Also happening on Linux, --> All/All
Comment 2 User image Jesse Ruderman 2002-05-24 20:26:44 PDT
*** Bug 146484 has been marked as a duplicate of this bug. ***
Comment 3 User image Pham 2003-08-01 21:37:44 PDT
.
Comment 4 User image Koike Kazuhiko 2003-11-15 23:47:07 PST
Here is an another case.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1871
Comment 5 User image Phil Ringnalda (:philor) 2005-09-16 22:44:20 PDT
*** Bug 308892 has been marked as a duplicate of this bug. ***
Comment 6 User image Masayuki Nakano [:masayuki] 2005-09-17 09:55:04 PDT
Created attachment 196429 [details] [diff] [review]
Patch rv1.0

This is simple fix. On nsEventStateManager::ChangeFocusWith, if the called
reason is by fired mouse, we should suppress scroll by focus. But we should not
break current suppressFocusScroll state. Therefore, I check current state and
recover old state.
Comment 7 User image Masayuki Nakano [:masayuki] 2005-09-17 09:59:19 PDT
This patch fixes bug 288945 and bug 300765 too.
Comment 8 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-17 20:32:08 PDT
It looks like if scroll-suppression is currently PR_TRUE we ignore it and still
scroll even if it was not a mouse event. That doesn't sound correct.

Also, the patch can be simplified by making the save-and-restore of the current
suppression status unconditional. Something like

PRBool savedSuppressScroll = ...;
if (!savedSuppressScroll && aFocusedWith == eEventFocusedByMouse) {
  SetSuppressFocusScroll(PR_TRUE);
}
...
SetSuppressFocusScroll(savedSuppressFocusScroll);

what do you think?
Comment 9 User image Masayuki Nakano [:masayuki] 2005-09-17 20:40:38 PDT
I think that we should suppress to scroll only by mouse click and clicked
element getting focus. If the element handles focus event and set focus to
another element, we should not suppress to scroll. I don't test this case using
|ChangeFocusWith|.
Comment 10 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-17 20:58:59 PDT
Okay, but can you make the save/restore simplification?
Comment 11 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-17 21:11:52 PDT
Comment on attachment 196429 [details] [diff] [review]
Patch rv1.0

looking for the simpler patch
Comment 12 User image Masayuki Nakano [:masayuki] 2005-09-17 21:12:03 PDT
Comment on attachment 196429 [details] [diff] [review]
Patch rv1.0

Sorry. I test my previous comment. |ChangeFocusWith| is not called by
javascript's set focus. Please wait. I will create simple patch.
Comment 13 User image Masayuki Nakano [:masayuki] 2005-09-17 21:38:19 PDT
Created attachment 196496 [details] [diff] [review]
Patch 1.1

Simple patch.
Comment 14 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-17 21:45:38 PDT
Comment on attachment 196496 [details] [diff] [review]
Patch 1.1

OK, but add an assertion before you change the state that the current state is
PR_FALSE.
Comment 15 User image Masayuki Nakano [:masayuki] 2005-09-17 21:49:27 PDT
(In reply to comment #14)
> (From update of attachment 196496 [details] [diff] [review] [edit])
> OK, but add an assertion before you change the state that the current state is
> PR_FALSE.
> 

You mean that 'before you change' is when I unlock scroll?
Comment 16 User image Masayuki Nakano [:masayuki] 2005-09-17 21:53:11 PDT
Created attachment 196498 [details] [diff] [review]
Patch for check-in

It this O.K.?
Comment 17 User image Masayuki Nakano [:masayuki] 2005-09-17 21:54:25 PDT
Created attachment 196499 [details] [diff] [review]
Patch for check-in

Sorry. here.
Comment 18 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-17 23:37:58 PDT
No, before you set it to PR_TRUE, check that it's currently PR_FALSE.
Comment 19 User image Masayuki Nakano [:masayuki] 2005-09-18 04:30:54 PDT
Created attachment 196518 [details] [diff] [review]
Patch for check-in
Comment 20 User image Masayuki Nakano [:masayuki] 2005-09-18 04:33:07 PDT
checked-in.
Comment 21 User image David Baron :dbaron: ⌚️UTC-8 2005-09-18 09:10:12 PDT
Um, shouldn't you be resetting it to what it used to be rather than to false? 
Wasn't that roc's point?
Comment 22 User image David Baron :dbaron: ⌚️UTC-8 2005-09-18 09:11:23 PDT
...or if it's really reliable that you're the only one changing this state,
shouldn't you keep the assertions in both places?
Comment 23 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-18 12:54:01 PDT
I think the first assertion is adequate.
Comment 24 User image Martijn Wargers [:mwargers] 2005-09-18 13:12:25 PDT
Ok, it works nicely in 2005-09-18 trunk build, but right-clicking a link still
exposes the bug. File a new bug or reopen this bug?
Comment 25 User image Hermann Schwab 2005-09-18 13:23:46 PDT
*** Bug 308919 has been marked as a duplicate of this bug. ***
Comment 26 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-18 13:39:46 PDT
Strange, right clicks appear to go through the same path. Maybe some JS UI code
is doing a DOM focus?

Anyway, file a separate bug.
Comment 27 User image Martijn Wargers [:mwargers] 2005-09-18 13:48:52 PDT
Ok, I filed bug 309075.
Comment 28 User image Frank Wein [:mcsmurf] 2005-11-15 15:17:48 PST
roc: Do you think this patch here is safe enough for 1.8 branch or is not needed/wanted (i personally don't think we want this on the branch)? Bug 316603 was filed about getting this patch into the branch.
Comment 29 User image David Baron :dbaron: ⌚️UTC-8 2005-11-15 15:19:55 PST
*** Bug 316603 has been marked as a duplicate of this bug. ***
Comment 30 User image Masayuki Nakano [:masayuki] 2005-11-15 15:40:04 PST
Now, we don't found any regressions. But I think we are already end of game for 1.5.
# if we go to this for 1.8 branch, go to bug 309075 together.
Comment 31 User image Alan 2005-11-23 08:07:42 PST
(In reply to comment #30)
> Now, we don't found any regressions. But I think we are already end of game for
> 1.5.
> # if we go to this for 1.8 branch, go to bug 309075 together.
> 

I first found this bug when I was testing Firefox 1.5RC3 on some sites.  If this isn't fixed before Firefox 1.5 is released this is going to be pretty bad.  ANY photo gallery site, porn site, intense graphics layout site, etc. that uses big images as links to the next image/page is going to be page shifting all over the place as users click between links.  It really makes the browser look buggy as things jump up and down between page loads.

Can anyone with more experience than me here please clarify if this can make it
to the Firefox 1.5 release?

Thanks.

Comment 32 User image Michael Newton 2005-11-29 14:19:43 PST
Can this get checked into the 1.8 branch?  It would be nice to have it there for the next 1.5.x release if possible!  It seems like a pretty trivial bit of code, with not much chance of causing any regressions.

I had filed a separate bug, but it was suggested I make a request in this bug instead.
Comment 33 User image Michael Newton 2005-12-01 17:13:38 PST
Created attachment 204750 [details] [diff] [review]
patch for 1.8 branch

I just learned that 2.0 won't even come off the trunk.  I'm not waiting for 3.0!  This patch incorporates the changes made in the previous patch for this bug, as well as bug 309075.

Never done a patch before, let's see if I mucked anything up!  Not sure if I'm supposed to set review? flag on this myself, or wait for one of the 'real' developers to do that...
Comment 34 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-02 17:56:28 PST
If you haven't really changed anything and it's just a diff of the same patch against the 1.8 branch, then we don't need any more review.

Also, requesting review from nobody will never get anywhere, so don't ever bother doing that.

You really want to nominate the patch for a branch, but currently we don't have a branch flag for what will become Firefox 2.0. I think drivers are still working on figuring out how that will work. Just keep an eye on this and come back to it in a few weeks when we'll have a solution hopefully, OK?
Comment 35 User image Martijn Wargers [:mwargers] 2005-12-03 02:00:47 PST
Just to (hopefully) make clear why it would be a good thing this would get on the 1.8 branch.
Bug 288945 was a regression from another bug in the 1.8 cycle, but got fixed by this fix. That bug already collected 5 duplicates.
Comment 36 User image Ben Basson 2005-12-03 06:45:38 PST
Nominating for 1.8.0.1 since fixing this in a point release would please plenty of people (bug 288945 can render some pages extremely difficult to use).

I'm intending to build from the branch with this patch - if anyone can give me pointers on possible areas of regression, I can try to keep an eye on things and report back if necessary. 
Comment 37 User image Ron 2005-12-20 12:01:32 PST
To give everyone a good example, here an IMG-map of the Benelux:

http://www.geocaching.nl/maps/seek.php

Put the mouse pointer somewhere in the north and click left and have a look at the result in FF 1.5.
Comment 38 User image Masayuki Nakano [:masayuki] 2005-12-21 10:56:23 PST
Comment on attachment 204750 [details] [diff] [review]
patch for 1.8 branch

O.K. The image map's issue is serious regression from Gecko 1.7, I think that we should fix this bug on 1.8 branch.
Comment 39 User image Daniel Veditz [:dveditz] 2006-01-05 12:00:40 PST
How can this be a regression from Gecko 1.7 if this bug was filed in 2001?

Ugly, but does't seem to fit the security/stability scope of 1.8.0.1, and adding more to that release just delays a quick 1.8.1
Comment 40 User image Daniel Veditz [:dveditz] 2006-01-05 12:01:34 PST
Comment on attachment 204750 [details] [diff] [review]
patch for 1.8 branch

Please do check in r/sr'd patches to the 1.8.1 branch, and use the fixed1.8.1 keyword
Comment 41 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-01-05 12:02:21 PST
(In reply to comment #39)
> How can this be a regression from Gecko 1.7 if this bug was filed in 2001?

Bug 288945 is the regression from Gecko 1.7, it was fixed by this bug.
Comment 42 User image Martijn Wargers [:mwargers] 2006-01-05 12:04:21 PST
(In reply to comment #39)
> How can this be a regression from Gecko 1.7 if this bug was filed in 2001?
See comment 35.
Comment 43 User image Michael Newton 2006-01-05 12:14:48 PST
(In reply to comment #39)
> How can this be a regression from Gecko 1.7 if this bug was filed in 2001?
> 
> Ugly, but does't seem to fit the security/stability scope of 1.8.0.1, and
> adding more to that release just delays a quick 1.8.1
> 

How can checking in a patch that's already been reviewed delay anything?  This is a big, easily visible regression that is affecting a fair number of users.  It would be nice if it were fixed in the next Firefox release.

BTW shouldn't this bug be reopened?
Comment 44 User image Martijn Wargers [:mwargers] 2006-01-05 12:18:26 PST
(In reply to comment #43)
> BTW shouldn't this bug be reopened?
No, fixed on trunk means the bug is fixed. 

Comment 45 User image Masayuki Nakano [:masayuki] 2006-01-06 06:12:37 PST
Daniel:

See comment 41.
And we don't have any regression repoarts by this bug.
I think that risk of this patch is very low.
Comment 46 User image Daniel Veditz [:dveditz] 2006-01-06 13:48:32 PST
(In reply to comment #40)
> Please do check in r/sr'd patches to the 1.8.1 branch, and use the fixed1.8.1
> keyword

I was incorrect, 1.8 branch checkins still require approval (which this patch got).
Comment 47 User image Daniel Veditz [:dveditz] 2006-01-10 11:54:34 PST
Comment on attachment 204750 [details] [diff] [review]
patch for 1.8 branch

a=dveditz for drivers
Comment 48 User image Jay Patel [:jay] 2006-01-11 17:14:00 PST
v.fixed on 1.8.0.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1, no scrolling for links partially off-screen.  Note, I used the latest build from this afternoon, so the checkin on the branch from this morning is in my build. :-)

Comment 49 User image Mike Palczewski 2006-02-18 10:46:59 PST
According to the changelog this bug was fixed in 1.5.0.1 of firefox.  I'm not getting this behavior anymore with the left click, but the middle click still causes it.  linux ubuntu stable official 1.5.0.1 firefox build
Comment 50 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-28 12:22:05 PDT
This broke various scripted scrolls; see bug 376785.  Masayuki, could you look into this, please?
Comment 51 User image Jesse Ruderman 2009-09-20 12:40:31 PDT
Clicking partially off-screen links is scrolling again.  I filed bug 517787 for the regression.

Note You need to log in before you can comment on or make changes to this bug.