Bug 25729 – Alt-clicking a link doesn't start a download

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

To reproduce:

1. Go to http://webkit.org/
2. Alt-click on "Install developer tools"

Nothing happens. The contents of the URL linked to by "Install developer tools" should be downloaded.

[Adam Roben (:aroben)]

<rdar://problem/6879075>

[Adam Roben (:aroben)]

I don't believe this is a regression.

There seem to be two parts to this bug:

1) WebKit isn't telling Safari that the Alt key is pressed when the link is hovered or clicked
   - This is due to a bug in PlatformKeyEvent
2) Once (1) is fixed, Safari tells WebKit to download the resource, but WebKit fails to start a download
  - This is due to WebFrame::startDownload being unimplemented

[Adam Roben (:aroben)]

Created attachment 30235 [details]
[1/3] refactor downloading code in WebKit

[Adam Roben (:aroben)]

Created attachment 30236 [details]
[2/3] Include MK_ALT in the modifier flags when the Alt key is pressed

[Adam Roben (:aroben)]

Created attachment 30237 [details]
[3/3] Implement WebFrame::startDownload

[Darin Adler]

Comment on attachment 30236 [details]
[2/3] Include MK_ALT in the modifier flags when the Alt key is pressed

Seems strange that we have to do this. Normally the job of PlatformEvent classes is to convert the event from the platform-specific to the platform-independent form. Having it change the modifier flags that came from the platform-native event seems outside its responsibilities. I'd expect clients to use m_altKey and not m_modifierFlags.

r=me though

[Adam Roben (:aroben)]

(In reply to comment #6)
> (From update of attachment 30236 [details] [review])
> Seems strange that we have to do this. Normally the job of PlatformEvent
> classes is to convert the event from the platform-specific to the
> platform-independent form. Having it change the modifier flags that came from
> the platform-native event seems outside its responsibilities. I'd expect
> clients to use m_altKey and not m_modifierFlags.

m_modifierFlags seems only to be used when calling ChromeClient::mouseDidMoveOverElement. We could get rid of m_modifierFlags entirely and have Chrome build up the set of flags itself based on the PlatformKeyEvent's other members (like m_altKey).

Thanks for the reviews!

[Darin Adler]

(In reply to comment #7)
> m_modifierFlags seems only to be used when calling
> ChromeClient::mouseDidMoveOverElement. We could get rid of m_modifierFlags
> entirely and have Chrome build up the set of flags itself based on the
> PlatformKeyEvent's other members (like m_altKey).

We should eliminate m_modifierFlags as soon as possible.

I'm not sure what the best way to pass these flags over to the ChromeClient is. Sending them over as a modifier flags word seems strange when that's the only place in all the code where we have the concept of modifier flags. If they are sent as a flags word, recipients might expect them to include various platform-specific flags. Maybe we can send something better than a flags word. Maybe the DOM mouse moved event? Or a set of booleans? I'd be hesitant to pass something over called modifierFlags that was similar but not the same to actual platform-specific modifier flags extracted from the platform-specific event.

[Adam Roben (:aroben)]

(In reply to comment #8)
> (In reply to comment #7)
> > m_modifierFlags seems only to be used when calling
> > ChromeClient::mouseDidMoveOverElement. We could get rid of m_modifierFlags
> > entirely and have Chrome build up the set of flags itself based on the
> > PlatformKeyEvent's other members (like m_altKey).
> 
> We should eliminate m_modifierFlags as soon as possible.

Sounds good to me. Luckily on the WebKit/mac and WebKit/win actually use the modifierFlags, so eliminating them shouldn't be too hard in terms of preserving existing behavior.

> I'm not sure what the best way to pass these flags over to the ChromeClient is.
> Sending them over as a modifier flags word seems strange when that's the only
> place in all the code where we have the concept of modifier flags. If they are
> sent as a flags word, recipients might expect them to include various
> platform-specific flags. Maybe we can send something better than a flags word.
> Maybe the DOM mouse moved event? Or a set of booleans? I'd be hesitant to pass
> something over called modifierFlags that was similar but not the same to actual
> platform-specific modifier flags extracted from the platform-specific event.

Passing the DOM event would probably require non-trivial code changes to WebCore. Passing the PlatformMouseEvent would be trivial. Passing a set of booleans would also be trivial, but seems less good than passing the PlatformMouseEvent. I agree that passing something called modifierFlags would not be ideal.

One potential problem is that m_modifierFlags contains information that PlatformMouseEvent's other members do not (e.g., only m_modifierFlags records whether NSFunctionKeyMask was included in the original NSEvent). If we need to preserve that information we'll probably need to add more members to PlatformMouseEvent (and MouseEvent, if we decide to pass a DOM event up to ChromeClient).

[Darin Adler]

(In reply to comment #9)
> One potential problem is that m_modifierFlags contains information that
> PlatformMouseEvent's other members do not (e.g., only m_modifierFlags records
> whether NSFunctionKeyMask was included in the original NSEvent). If we need to
> preserve that information we'll probably need to add more members to
> PlatformMouseEvent (and MouseEvent, if we decide to pass a DOM event up to
> ChromeClient).

There seem to be two options here:

    1) Preserve any needed information like NSFunctionKeyMask in a specific more-platform-independent way in the PlatformEvent object.

    2) Preserve the original actual raw platform-specific event and have the client get at that through the PlatformEvent.

I slightly prefer (1) over (2).

A separate issue is that over time I'd love to see the DOM event object be the only one clients have to deal with. PlatformEvent seems more suitable for internal use but I'd prefer it not be used in the WebKit level (just in WebCore). One advantage of just passing a modifier flags word is that it exposes less of the PlatformEvent class details, which is good if we want to keep it out of the WebKit level.

[Oliver Hunt]

Have these patches been landed?

[Adam Roben (:aroben)]

(In reply to comment #11)
> Have these patches been landed?

They haven't. I'll make sure to land them this week.

[Adam Roben (:aroben)]

Comment on attachment 30236 [details]
[2/3] Include MK_ALT in the modifier flags when the Alt key is pressed

After thinking some more about Darin's comments in comment 6, I've decided it's probably better to keep m_modiferFlags exactly the same as what was received in the WM_MOUSEMOVE event. Then the client (i.e., Safari) can just handle checking for Alt on its own.

[Adam Roben (:aroben)]

Landed as r44271 and r44273.

http://trac.webkit.org/changeset/44271
http://trac.webkit.org/changeset/44273

[Adam Roben (:aroben)]

(In reply to comment #13)
> (From update of attachment 30236 [details] [review])
> After thinking some more about Darin's comments in comment 6, I've decided it's
> probably better to keep m_modiferFlags exactly the same as what was received in
> the WM_MOUSEMOVE event. Then the client (i.e., Safari) can just handle checking
> for Alt on its own.

It turns out that with just landing attachment 30235 [details] and attachment 30237 [details], Safari now downloads in response to alt-click. What it still *doesn't* do is show the "Download 'http://www.example.com/'" text in the status bar. That is a different bug, and is being tracked by <rdar://problem/6933644>.

So, this bug is now entirely fixed, requiring no changes in Safari.