255378 - Click-and-hold on subsequent lines of wrapped links fails to invoke context menu

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Wayne Woods]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040811 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040811 Firefox/0.9.1+

I've only tested this in Mac OS X so far. I'll test it out in Windows tomorrow.

On the Mac, if you click-and-hold on a link, it brings up a context menu with
options such as "Open Link in New Window "(same as right click on Windows).

In recent builds, if the link is long and wraps over two or more lines, then
hold-clicking on any line of the link *except* the first line often fails to
bring up the context menu. Hold-clicking on the first line of the link will
always bring up the menu, however. If you've brought up the context menu by
hold-clicking on the first line, then it usually works on subsequent lines for a
while. If you change the focus, say by highlighting a word, then the subsequent
lines become unresponsive once again.

This regression occurred between the 20040714 and 20040715 builds, at least in
Firefox. The problem exists in Seamonkey as well, however.

I'll attach a simple test case so the problem can be tested easily.

Reproducible: Always
Steps to Reproduce:
1.Navigate to a page with a link that is fairly long (say, a whole sentence).
See the attached test case.
2.Shrink the browser window until the link is forced to wrap over more than one
line.
3.Click and hold on the second line of the link.
4.Observe that no context menu appears (hopefully). If the menu does appear,
then dismiss it and highlight a section of text that is outside the link, and
try again.

Comment 1

[Wayne Woods]

Attachment: Contains a long link that helps in reproducing the bug

Open the attachment, then shrink the browser window to be sure that the link
wraps over multiple lines. Click-and-hold on any line except the first one and
observe the lack of context menu. If it does appear, then try highlighting some
text and then try again.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> This regression occurred between the 20040714 and 20040715 builds

What are the timestamps on those builds?

Comment 3

[Wayne Woods]

I was never sure how to determine the timestamp for a Fx build, as it doesn't
list it in the about: info. Where can I find it?

I can download a few Seamonkey builds and test them out, anyway.

Comment 4

[Boris Zbarsky [:bzbarsky]]

It's not in the URL bar?  What about the filename on the FTP site?

Comment 5

[Wayne Woods]

I presume you mean the title bar, which is where Seamonkey lists build id? If
so, Firefox doesn't do that. The FTP file never lists build id either. I'm not
sure where else to look. Would there be some way of telling by looking at the
Tinderbox for those dates, or something similar?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> The FTP file never lists build id either.

Er... Not if you look in latest/, but if you look in the dated directories...

Comment 7

[Wayne Woods]

Does the ftp directory name match the true timestamp, though? I remember looking
at Seamonkey builds in the past, and the 2 digits after the date for the
directory didn't match the 2 digits after the date that appeared on the title bar.

Comment 8

[Boris Zbarsky [:bzbarsky]]

It's usually within an hour or two.. That usually gives a window of 27-28 hours,
which is a lot less than the 48 you get with just dates.

Comment 9

[Wayne Woods]

Okay, I'll download them again just to be sure and test them out tonight
(tonight being another 6-7 hours away for me). I'll download some Seamonkey
builds from the same time period and note the timestamps for all.

Incidentally, right-clicking in Windows XP doesn't produce the same bug, so it
looks like it's Mac-specific.

Comment 10

[Wayne Woods]

Okay, the last trunk builds to work properly were:
Firefox 2004071409
Mozilla 2004071408
The first ones to exhibit this bug were:
Firefox 2004071509
Mozilla 2004071508
The Mozilla Seamonkey timestamps were taken from the title bar, and the Firefox
timestamps were taken from the directory name.

Of interest, I tried a number of branch builds of both Seamonkey (1.7 branch)
and Firefox (0.9 branch), and none exhibit this bug. The latest branch build I
tested was the 0.9.3 release of Firefox.

So whatever checkin caused this regression, it was only checked into the trunk.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Of the checkins in the relevant range
(http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-07-14+08%3A00%3A00&maxdate=2004-07-15+08%3A00%3A00&cvsroot=%2Fcvsroot),
 the ones that look like they may be responsible are:

bug 65917, bug 151375 (the latter is what I really suspect).

Wayne, could you test something for me?  Remove the 

  *|*:-moz-any-link:focus {
   -moz-outline: 1px dotted invert;
  }

rule from your ua.css file (with Mozilla shut down) and retest this bug?  Does
that help any?

Comment 12

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

Sounds like reflowing the element because it got focus is killing the hold. I
wonder why, no frames should be destroyed.

Comment 13

[Aaron Leventhal]

> Sounds like reflowing the element because it got focus is killing the hold. I
> wonder why, no frames should be destroyed.
We have to reframe when the outline gets drawn, because there's no other change
hint to force creation of a view without doing that. That's also causing bug
257672. In that bug Roc said I was wrong about the fix, but I still think it
would be fixed by bug 249102

Comment 14

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

No, we don't force view creation.
http://lxr.mozilla.org/mozilla/source/content/shared/src/nsStyleStruct.cpp#641

Comment 15

[Aaron Leventhal]

(In reply to comment #14)
> No, we don't force view creation.
> http://lxr.mozilla.org/mozilla/source/content/shared/src/nsStyleStruct.cpp#641

My understanding is that we would just force view creation / removal when an
outline appears/disappears, but the only way to do that right now is to force
frame reconstruction. I thought that was the idea of fixing bug 249102, although
now that I read nsChangeHint.h I'm not sure that's what
nsChangeHint_ForceFrameView is supposed to do
(http://lxr.mozilla.org/seamonkey/source/content/shared/public/nsChangeHint.h#50)
> 50   // TBD: add nsChangeHint_ForceFrameView to force frame reconstruction if
the frame doesn't yet
> 51   // have a view

Comment 16

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

We don't force frame reconstruction either. Read the code. All we do is a reflow.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm sure the click-hold thing has something set up to make it go away if the
mouse moves... could the reflow be synthesizing mouse move events or something?

ccing pinkerton, since he knows something about how click-hold works, iirc.

Comment 18

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

That sounds likely

Comment 19

[Wayne Woods]

I couldn't find the ua.css file (assuming the test is still useful at this
point). Where's it kept?

Comment 20

[Mike Pinkerton (not reading bugmail)]

i forget if the click hold is stopped by a mouse move alone. it is stopped by a
synthesized drag_gesture event which is made up from mouse moves. all the
click-hold code is in the ESM, it's not very complicated. basically an event
handler and a timer.

Comment 21

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #19)
> I couldn't find the ua.css file. Where's it kept?

res/ua.css in the Mozilla install dir.

Comment 22

[Wayne Woods]

Removing:

  *|*:-moz-any-link:focus {
   -moz-outline: 1px dotted invert;
  }

from ua.css does indeed fix the problem, in both Mozilla and Firefox.

Comment 23

[Wayne Woods]

Setting a blocking request just to make it visible again :) The cause of this
bug has been known for quite a while now, see comment 22, so hopefully someone
knows what to do. :)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, man.  First, this is in the wrong product.  Second, the cause is NOT known.
 Just removing that CSS is not an option, and the CSS is not the cause of the
bug.  The cause of the bug is something in the interaction between the
click-hold listener and layout and we (or at least I) don't really know what.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Nominating for the relevant near-term releases (most importantly, the Gecko 1.8
nomination).

Comment 26

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

Attachment: fix

Similar fix to what we did to make dragging work in the face of frame
destruction/reconstruction (which happens here because of inline frame
pullup/pushback). In fact, we can reuse most of that work and simplify things a
lot.

Two notes: using mPresContext is fine, we cannot be dispatching events to
content that's not in this ESM's document. I'm changing the behaviour of drag
events so that they specify the keyboard modifiers at the time of original
mousedown, but I think that actually makes more sense than using the modifiers
from when the mouse first moved enough for a drag to start.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181004 [details] [diff] [review]
fix

>Index: content/events/src/nsEventStateManager.cpp
> nsEventStateManager::KillClickHoldTimer()
> } // KillTooltipTimer

Want to fix that comment?

>+nsEventStateManager::FillInEventFromGestureDown(nsMouseEvent* aEvent,

>+  nsIView* view = mCurrentTarget->GetClosestView(&targetToView);
>+  nsPoint viewToWidget;
>+  nsIWidget* widget = view->GetNearestWidget(&viewToWidget);
>+  NS_ASSERTION(widget == aEvent->widget, "Widget confusion");
>+  aEvent->point = refPointTwips - (targetToView + viewToWidget);

This is putting the point in the coord system of the _frame_ we're dispatching
to, no?  Is that really right?	Is that fixed up somewhere else to the correct
view's coord system?  I know you just copied this code, but it seems fishy. 
Possibly separate bug fodder.

>Index: content/events/src/nsEventStateManager.h
>+  void FillInEventFromGestureDown(nsMouseEvent* aEvent, PRBool aIsTrusted);

Document a bit, please.

r+sr=bzbarsky with that.

Comment 28

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

Attachment: fix #2

Here's the new patch incorporating your comments. I've fixed the coordinate
translation.

Comment 29

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

Attachment: oops

That patch enabled click-hold context menus on all platforms, we really don't
want that :-).

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181076 [details] [diff] [review]
oops

r+sr=bzbarsky

Comment 31

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

Comment on attachment 181076 [details] [diff] [review]
oops

This fixes a 1.8b3 blocker

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 181076 [details] [diff] [review]
oops

a=asa

Comment 33

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

checked in

Comment 34

[Jon C. Thomason]

I believe this checkin is responsible for Camino crashing bug 299419.