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

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
Event Handling
P3
minor
VERIFIED FIXED
16 years ago
3 years ago

People

(Reporter: Matthew Paul Thomas, Assigned: masayuki)

Tracking

({verified1.8.0.1, verified1.8.1})

Trunk
mozilla1.8.1
verified1.8.0.1, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.0.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Image map's issue(bug 288945) is serious regression from Gecko 1.7.)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
Also happening on Linux, --> All/All
Severity: normal → minor
OS: Mac System 9.x → All
Hardware: Macintosh → All

Updated

16 years ago
Target Milestone: --- → Future

Updated

15 years ago
QA Contact: madhur → rakeshmishra

Comment 2

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

Updated

15 years ago
QA Contact: rakeshmishra → trix

Comment 3

14 years ago
.
Assignee: joki → saari
QA Contact: trix → ian

Comment 4

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

Updated

12 years ago
Blocks: 288945
(Assignee)

Updated

12 years ago
Assignee: saari → events

Updated

12 years ago
Blocks: 300765
*** Bug 308892 has been marked as a duplicate of this bug. ***
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.
Assignee: events → masayuki
Status: NEW → ASSIGNED
Attachment #196429 - Flags: superreview?(roc)
Attachment #196429 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Priority: -- → P3
Target Milestone: Future → mozilla1.9alpha
This patch fixes bug 288945 and bug 300765 too.
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?
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|.
Okay, but can you make the save/restore simplification?
Comment on attachment 196429 [details] [diff] [review]
Patch rv1.0

looking for the simpler patch
Attachment #196429 - Flags: superreview?(roc)
Attachment #196429 - Flags: superreview-
Attachment #196429 - Flags: review?(roc)
Attachment #196429 - Flags: review-
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.
Attachment #196429 - Flags: superreview-
Created attachment 196496 [details] [diff] [review]
Patch 1.1

Simple patch.
Attachment #196429 - Attachment is obsolete: true
Attachment #196496 - Flags: superreview?(roc)
Attachment #196496 - Flags: review?(roc)
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.
Attachment #196496 - Flags: superreview?(roc)
Attachment #196496 - Flags: superreview+
Attachment #196496 - Flags: review?(roc)
Attachment #196496 - Flags: review+
(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?
Created attachment 196498 [details] [diff] [review]
Patch for check-in

It this O.K.?
Created attachment 196499 [details] [diff] [review]
Patch for check-in

Sorry. here.
Attachment #196498 - Attachment is obsolete: true
No, before you set it to PR_TRUE, check that it's currently PR_FALSE.
Created attachment 196518 [details] [diff] [review]
Patch for check-in
checked-in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Um, shouldn't you be resetting it to what it used to be rather than to false? 
Wasn't that roc's point?
...or if it's really reliable that you're the only one changing this state,
shouldn't you keep the assertions in both places?
I think the first assertion is adequate.
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

12 years ago
*** Bug 308919 has been marked as a duplicate of this bug. ***
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.
Ok, I filed bug 309075.
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.
*** Bug 316603 has been marked as a duplicate of this bug. ***
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

12 years ago
(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

12 years ago
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

12 years ago
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...

Updated

12 years ago
Blocks: 318757

Updated

12 years ago
Attachment #204750 - Flags: review?
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?

Updated

12 years ago
No longer blocks: 318757
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

12 years ago
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. 
Flags: blocking1.8.0.1?

Comment 37

12 years ago
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 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.
Attachment #204750 - Flags: superreview+
Attachment #204750 - Flags: review?
Attachment #204750 - Flags: review+
Attachment #204750 - Flags: approval1.8.0.1?
(Assignee)

Updated

12 years ago
Whiteboard: Image map's issue(bug 288945) is serious regression from Gecko 1.7.
(Assignee)

Updated

12 years ago
Attachment #204750 - Flags: approval1.8.1?
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
Flags: blocking1.8.0.1? → blocking1.8.0.1-
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
Attachment #204750 - Flags: approval1.8.1? → approval1.8.1+
(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.
(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

12 years ago
(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?
(In reply to comment #43)
> BTW shouldn't this bug be reopened?
No, fixed on trunk means the bug is fixed. 

(Assignee)

Updated

12 years ago
Keywords: fixed1.8.1
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.
Flags: blocking1.8.0.1- → blocking1.8.0.1?
Target Milestone: mozilla1.9alpha → mozilla1.8.1
(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).
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
Keywords: qawanted
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1-
Flags: blocking1.8.0.1+
Comment on attachment 204750 [details] [diff] [review]
patch for 1.8 branch

a=dveditz for drivers
Attachment #204750 - Flags: approval1.8.0.1? → approval1.8.0.1+
(Assignee)

Updated

12 years ago
Keywords: fixed1.8.0.1

Comment 48

12 years ago
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. :-)

Keywords: fixed1.8.0.1 → verified1.8.0.1
(Assignee)

Updated

12 years ago
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1

Comment 49

12 years ago
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
Depends on: 376785
This broke various scripted scrolls; see bug 376785.  Masayuki, could you look into this, please?

Comment 51

8 years ago
Clicking partially off-screen links is scrolling again.  I filed bug 517787 for the regression.
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.