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] (Mozilla Japan) (working slowly due to injured)
: Hixie (not reading bugmail)
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] (Mozilla Japan) (working slowly due to injured)
roc: review-
Details | Diff | Review
Patch 1.1 (2.25 KB, patch)
2005-09-17 21:38 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
roc: review+
roc: superreview+
Details | Diff | Review
Patch for check-in (2.40 KB, patch)
2005-09-17 21:53 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch for check-in (2.40 KB, patch)
2005-09-17 21:54 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch for check-in (2.41 KB, patch)
2005-09-18 04:30 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | 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 | Review

Description 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 Matthew Paul Thomas 2001-10-21 04:45:37 PDT
Also happening on Linux, --> All/All
Comment 2 Jesse Ruderman 2002-05-24 20:26:44 PDT
*** Bug 146484 has been marked as a duplicate of this bug. ***
Comment 3 Pham 2003-08-01 21:37:44 PDT
.
Comment 4 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 Phil Ringnalda (:philor) 2005-09-16 22:44:20 PDT
*** Bug 308892 has been marked as a duplicate of this bug. ***
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 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 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2005-09-17 09:59:19 PDT
This patch fixes bug 288945 and bug 300765 too.
Comment 8 Robert O'Callahan (:roc) (Exited; 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 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 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 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-17 20:58:59 PDT
Okay, but can you make the save/restore simplification?
Comment 11 Robert O'Callahan (:roc) (Exited; 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 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 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 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2005-09-17 21:38:19 PDT
Created attachment 196496 [details] [diff] [review]
Patch 1.1

Simple patch.
Comment 14 Robert O'Callahan (:roc) (Exited; 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 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 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 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2005-09-17 21:53:11 PDT
Created attachment 196498 [details] [diff] [review]
Patch for check-in

It this O.K.?
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2005-09-17 21:54:25 PDT
Created attachment 196499 [details] [diff] [review]
Patch for check-in

Sorry. here.
Comment 18 Robert O'Callahan (:roc) (Exited; 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 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2005-09-18 04:30:54 PDT
Created attachment 196518 [details] [diff] [review]
Patch for check-in
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2005-09-18 04:33:07 PDT
checked-in.
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 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 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 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 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-18 12:54:01 PDT
I think the first assertion is adequate.
Comment 24 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 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 Hermann Schwab 2005-09-18 13:23:46 PDT
*** Bug 308919 has been marked as a duplicate of this bug. ***
Comment 26 Robert O'Callahan (:roc) (Exited; 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 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-09-18 13:48:52 PDT
Ok, I filed bug 309075.
Comment 28 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 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-15 15:19:55 PST
*** Bug 316603 has been marked as a duplicate of this bug. ***
Comment 30 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 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 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 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 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 Robert O'Callahan (:roc) (Exited; 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 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 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 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 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 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 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 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 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 :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 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 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 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 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 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 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 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 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 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 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 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 Boris Zbarsky [:bz] 2009-04-28 12:22:05 PDT
This broke various scripted scrolls; see bug 376785.  Masayuki, could you look into this, please?
Comment 51 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.