215926 - Conflicting shortcuts: Alt+drag selects part of a link, Alt+click opens link

Status: RESOLVED WORKSFORME

(Firefox :: General, defect)

Description

[Donny]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.5b) Gecko/20030810 Mozilla Firebird/0.6.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.5b) Gecko/20030810 Mozilla Firebird/0.6.1+

You can save page by holding down alt + clicking on the hyperlink, this would
bring up save as dialog. You can also select part of the hyperlink (ex. link:
"Page www.mozilla.org looks nice" and you want to select: "www.mozilla.org" with
a help of mouse) by holding down alt and selecting it.

Some problem for Linux+KDE users:
http://forums.mozillazine.org/viewtopic.php?p=149100#149100

PS. Using Windows 2003, you could add it to OS selection list

Reproducible: Always

Steps to Reproduce:
1.Find hyperlink
2.Hold down alt
3.Select a part of hyperlink using your mouse
Actual Results:  
you get 'save as' dialog

Expected Results:  
Simply selected part of the hyperlink text

Maybe change alt to ctrl for hyperlink selection?

Comment 2

[Stuart Morgan]

This is core behavior, and affects more than just Firefox, so -> Core

Comment 3

[blackwind]

Attachment: Patch to ensure saveURL() only fires if no selection has been made

Comment 4

[blackwind]

Attachment: Patch to ensure saveURL() only fires if no selection has been made

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 559831 [details] [diff] [review]
Patch to ensure saveURL() only fires if no selection has been made

Thanks for the patch! I think it might be unexpected behavior of Alt+Click to not save the target when some text happens to be selected. Bug 378775 might be a better way to solve the conflict between these features. I'm changing the review request to a ui-review request to get this evaluated.

Comment 6

[blackwind]

Because Alt+Clicking a link also deselects any text that was previously selected, though, saveURL() will only be suppressed if the user purposely selects text within the link being clicked; therefore, this should be an acceptable fix. If anything, I'd classify the *current* behavior as unexpected. Here's hoping the UI team agrees!

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

>I think it might be unexpected behavior of Alt+Click to not save the target when some text >happens to be selected.

If the user is holding alt and doing a text selection, I don't think they necessarily have any clear intention of actually wanting the download.  We could eventually fix Bug 378775 to deal with the conflict between these features, and this bug would otherwise still exist, so let's move forward with fixing at least this one.

Comment 8

[Vova Olar]

As this is reviewed, when it will be landed?

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It would be great of you could produce a patch as described in <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F>.

Comment 10

[Ed Morley [:emorley]]

(In reply to Vova Olar from comment #8)
> As this is reviewed, when it will be landed?

This has only had a ui-review, it needs a normal review too before it can be landed; sorry for any confusion. (I'll request that review now).

Comment 11

[Ed Morley [:emorley]]

Dao, ping for review on behalf of blackwind, a first time patch contributor who has just asked on #developers about the review status of this patch.

Thanks :-)

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 559831 [details] [diff] [review]
Patch to ensure saveURL() only fires if no selection has been made

I don't think getBrowserSelection is going to work in multi-process Firefox and I'm hesitant to take a patch that will break in the not-too-distant future. However, I don't even know how the current unpatched function is going to work.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 559831 [details] [diff] [review]
Patch to ensure saveURL() only fires if no selection has been made

If getBrowserSelection was the only problem for e10s here we could make it an async callback pretty easily. However, all of handleLinkClick and contentAreaClick will require more effort (probably will need to move contentAreaUtils code to a framescript), so I don't see a problem with taking this patch for now.

It'd be nice to have a test to cover this change.. I don't know how to test for the saveURL function though (to know if it was called or not called as expected)

Comment 14

[blackwind]

Can we move forward with this, then? We're now on month two for a one-line patch.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 559831 [details] [diff] [review]
Patch to ensure saveURL() only fires if no selection has been made

I can't get this patch applied. Did you use Mercurial (https://developer.mozilla.org/en/Mercurial) to generate this?

Comment 16

[Ed Morley [:emorley]]

blackwind, to make it possible for dao to apply the patch and test it, can you attach a patch in the format mentioned in comment 9 please.

Thanks :-)

Comment 17

[blackwind]

Attachment: Patch to ensure saveURL() only fires if no selection has been made

Here's the patch in Mercurial format. Hope this one's sufficient.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 575691 [details] [diff] [review]
Patch to ensure saveURL() only fires if no selection has been made

This one works. Thanks!

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/22f61e71c108

Comment 20

[Matt Brubeck (:mbrubeck)]

Backed out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40cb4adf09cc

because of test failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7518312&tree=Mozilla-Inbound
browser_contentAreaClick.js | Alt click on XLinks:saveURL was invoked - Didn't expect -1, but got it

Comment 21

[blackwind]

So, what now?

Comment 22

[Matt Brubeck (:mbrubeck)]

(In reply to blackwind from comment #21)
> So, what now?

In order to land this fix, the test failures will need to be fixed.  To diagnose and fix the failures, you can run the tests yourself [1] or someone with commit access can push them to the try server [2] for you.  Once you have a version of the patch that passes all tests, you can request review and checkin again.

1. https://developer.mozilla.org/en/Mozilla_automated_testing
2. https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 23

[blackwind]

This is the point, then, at which I bail.

To those who've been following this bug for a solution, an interim one can be found here:

https://userscripts.org/scripts/show/118883

Hope to see a proper fix landed someday. Cheers!

Comment 24

[Marco Bonardo [:mak]]

So, looks like on the svgxlink test getBrowserSelection() returns something and saveURL is skipped, but on the previous mathxlink test getBrowserSelection() is an empty string and saveURL is executed. I suppose what is in the selection may be the content of the SVG Xlink, but I'm not sure how it would finish there, unless clicking does not put the clicked object in the selection for a while, and in this case the text would be extracted from the svg

Comment 25

[Marco Bonardo [:mak]]

Indeed, if I alt click on the SVG link test, and I check getBrowserSelection() in Scratchpad, it contains "SVG XLink" that is exactly its text node.

Comment 26

[[:Aleksej]]

Related: bug 50673.

Comment 27

[Sid]

Not an issue since bug 713052 landing.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Indeed, fixed by bug 713052

Comment 29

[blackwind]

When you alt+drag, Firefox 13+ navigates to the link being selected on mouseup. This bug may be fixed in the sense that there are no longer conflicting shortcuts, but the functionality itself is still completely broken.

Comment 30

[Michal 'hramrach' Suchanek]

Can you explain how is this fixed?

I still canot select link text in Iceweasel 21. The damn browser opens the link instead.

Comment 31

[:Gijs (he/him)]

We tried to reproduce this on both Windows and macOS, but it seems fixed. If people are still seeing this, please provide more detailed STR before reopening.