268830 - Popup blocker wrongly blocks when a link is selected by an access key

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Bernard sumption]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

On http://www.fastmail.fm/ there are links that open in a new window (set
through a target attribute), that have access keys (set through the accesskey
attribute). There are no JavaScript events on them.

If you either click the link, or bring the link into focus and hit return, the
link opens as normal in a new window. If you try and activate the link by typing
the access key (e.g. ctrl-F for 'FAQ'), the popup blocker blocks the new window.

Reproducible: Always
Steps to Reproduce:
1. Go to a page with accesskey links that target a new window.
2. Use the access key

Actual Results:  
A blocking notice appeared.

Expected Results:  
Window opened

Comment 1

[Phil Ringnalda (:philor)]

Also blocks in Seamonkey and Firefox trunk on Windows. -> All/All/Browser.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Phil, please do NOT confirm bugs while leaving them in browser-general and not
ccing anyone.  That's the best way to make sure no one ever looks at the bug
again...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the problem here is that keypress events are not in
"dom.popup_allowed_events".  So the code in
nsDOMEvent::GetEventPopupControlState ends up with a openAbused for the state,
and we block the popup.

I'm not sure whether we need to add keypress to dom.popup_allowed_events (as
click is, basically), or whether we need to hack something into the accesskey
code...

Comment 5

[Wladimir Palant]

This is supposed to be fixed - see bug 268830 comment 70. jst solved it by
forwarding event flags from keypress events to resulting click events. Seems to
be some recent regression, bug 265176 is my favorite so far without looking at
anything too closely...

Comment 6

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #5)
> This is supposed to be fixed - see bug 268830 comment 70.

Wrong bug number?  there is no comment 70 in this bug...

> bug 265176 is my favorite so far

I checked that before commenting.  The key event is properly marked trusted, and
so is the resulting click event (see
http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#1018).

Hence my guess that the issue is with GetEventPopupControlState.  Then I tested
that adding "keypress" to that pref fixes this bug (it does).

Comment 7

[Wladimir Palant]

Sorry, I meant bug 252326 comment 70 of course...

The strange thing is - only popups from href are blocked, popups from onclick
still work. That's why I guessed bug 265176 caused this.

Comment 8

[Wladimir Palant]

I backed out the patch from bug 265176 and recompiled - it still doesn't work,
so it has to be something else...

Comment 9

[Boris Zbarsky [:bzbarsky]]

> The strange thing is - only popups from href are blocked, popups from onclick
> still work.

Before we run the onclick JS handler, we push the popup control state for
"onclick" (see nsEventListenerManager::HandleEvent).  Perhaps we should do the
same in the accesskey code?  That feels like such a nasty hack, but...

Comment 10

[Boris Zbarsky [:bzbarsky]]

We really need to fix this.  Silently ignoring accesskeys is bad...

Comment 11

[Phil Ringnalda (:philor)]

Is opening a bookmark when you type the first letter (if it's unique in the
folder) implemented as an actual accesskey through the same code, making bug
275071 a dup of this?

Comment 12

[Asa Dotzler [:asa]]

Johnny, can you have a look at this? 

Comment 13

[Johnny Stenback (:jst)]

Attachment: Fix.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 170420 [details] [diff] [review]
Fix.

Oh, du.  This works, of course.

Requesting 1.8a6 approval.  This is a pretty straightforward patch.

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 170420 [details] [diff] [review]
Fix.

a=asa for checkin to 1.8a6.

Comment 16

[Johnny Stenback (:jst)]

Fixed.