105894 - Clicking a partially off-screen link shouldn't scroll the page

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Matthew T (active 1999-2002)]

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 T (active 1999-2002)]

Also happening on Linux, --> All/All

Comment 2

[Jesse Ruderman]

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

Comment 3

[Pham]

.

Comment 4

[Koike Kazuhiko]

Here is an another case.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1871

Comment 5

[Phil Ringnalda (:philor)]

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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: 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] (he/him)(JST, +0900)]

This patch fixes bug 288945 and bug 300765 too.

Comment 8

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

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] (he/him)(JST, +0900)]

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) (email my personal email if necessary)]

Okay, but can you make the save/restore simplification?

Comment 11

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

Comment on attachment 196429 [details] [diff] [review]
Patch rv1.0

looking for the simpler patch

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

Attachment: Patch 1.1

Simple patch.

Comment 14

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

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] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

Attachment: Patch for check-in

It this O.K.?

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for check-in

Sorry. here.

Comment 18

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

No, before you set it to PR_TRUE, check that it's currently PR_FALSE.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for check-in

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

...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) (email my personal email if necessary)]

I think the first assertion is adequate.

Comment 24

[Martijn Wargers (dead)]

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]

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

Comment 26

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

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 (dead)]

Ok, I filed bug 309075.

Comment 28

[Frank Wein [:mcsmurf]]

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-4, no longer working on Mozilla)]

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

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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]

(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]

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]

Attachment: 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) (email my personal email if necessary)]

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 (dead)]

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]

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]

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] (he/him)(JST, +0900)]

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]]

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]]

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: [e-mail]]]

(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 (dead)]

(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]

(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 (dead)]

(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] (he/him)(JST, +0900)]

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]]

(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]]

Comment on attachment 204750 [details] [diff] [review]
patch for 1.8 branch

a=dveditz for drivers

Comment 48

[Jay Patel [:jay]]

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]

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 [:bzbarsky]]

This broke various scripted scrolls; see bug 376785.  Masayuki, could you look into this, please?

Comment 51

[Jesse Ruderman]

Clicking partially off-screen links is scrolling again.  I filed bug 517787 for the regression.