414628 - right clicking on links in addons manager acts as a left click

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012904 Minefield/3.0b3pre ID:2008012904

Go into the Tools > Addons > Get Add-ons UI and there are a bunch of links that can appear in there:
- Browse All Addons
- Learn More
- See All Results (xx)
- See All Recommended Add-ons
- etc

Right clicking on any of these links acts as if you have left-clicked them, and takes you to the target URL.

Comment 1

[Robert Strong (they/them - no direct email)]

Likely a widget bug since other links such as "Get more search engines..." in the app do the same thing.

Comment 2

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

(In reply to comment #1)
> Likely a widget bug

Indeed. The text-link binding in text.xml uses an onClick handler and doesn't check the button.

Comment 3

[Jesse Ruderman]

Dup of bug 419302?

Comment 5

[Régis Caspar]

Attachment: patch

Patch removing the onclick attributes from the text-link and adding a xbl:inherits to set the href attribute. For 2 text-link in extension.xul, the openURL has been conditioned by a "if (event.button == 0)".

Tested on a 2008080310 home made build.

Comment 6

[Robert Strong (they/them - no direct email)]

Regis, could a fix to text.xml's text-link binding be done instead? There are other consumers of this that have the same bug and it would likely be better if the fix was done there instead.

Comment 7

[Régis Caspar]

Robert, the problem is the way those text-link are used in ext manager, the use of onclick in extension.xml is the problem because this doesn't check which button is clicked. In text.xml, when using href, the click event checks the button, that's why I used xbl:inherits to propagate href. 
So I don't see how this can be fixed in text.xml.

Comment 8

[Robert Strong (they/them - no direct email)]

Thanks Regis... could you also fix the same bug for the about dialog as well?
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/about.xul#65

Comment 9

[Régis Caspar]

Attachment: patch v2

Updated patch fixing about.xul for extensions too.

tested on the same build as previous patch (rebuild of course)

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 332758 [details] [diff] [review]
patch v2

r=me and thanks Regis

Comment 11

[Robert Strong (they/them - no direct email)]

Files bug 449622 for search

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 332758 [details] [diff] [review]
patch v2

Regis, can you verify that this doesn't break respecting the pref to open a new tab vs. to open a new window. If it doesn't openURL will need to be used and this patch will need to be revised / re-reviewed. Thanks

Comment 13

[Régis Caspar]

No it follow the pref (as before ?)

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 332758 [details] [diff] [review]
patch v2

I just verified that the Learn More link in the Get Add-ons view opens a new window with this patch when Tools -> Options -> Tabs -> New pages should be opened in -> a new tab is selected. openURL needs to be used which is why the bindings that could have used href used onclick instead.

Comment 15

[Régis Caspar]

Attachment: patch v3

Oh I forgot to check the href ones! Here's the correct patch (using the if test for everything), sorry.

Comment 16

[Robert Strong (they/them - no direct email)]

Comment on attachment 332777 [details] [diff] [review]
patch v3

No worries and thanks for the patch

Comment 17

[Régis Caspar]

Thanks for the review. 

Perhaps we should make text-link open() obey the tab/new window pref too (in another bug), what do you think?

Comment 18

[Robert Strong (they/them - no direct email)]

(In reply to comment #17)
> Thanks for the review. 
> 
> Perhaps we should make text-link open() obey the tab/new window pref too (in
> another bug), what do you think?
That's Bug 263433

Comment 19

[Dão Gottwald [:dao]]

Can't we add a function for this to extensions.js? Something like:

function openLabelURL(event, attributeName) {
  if (event.button == 0)
    openURL(event.target.getAttribute(attributeName));
}

Comment 20

[Robert Strong (they/them - no direct email)]

Of course we could but it would be better to figure out a decent way to make the widget itself fulfill this functionality so I'm not very concerned with doing so.

Comment 21

[Régis Caspar]

In fact this is temporary, until bug 263433 is fixed then we can use the href property.

Comment 22

[Dão Gottwald [:dao]]

Ok, I guess the right thing is to file a new bug for cleaning this stuff up and make it depend on bug 263433. Régis, can you do this? I'll land your patch later today.

Comment 23

[Régis Caspar]

Dão, I filled bug 450642 for that (back-out and use of href).

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/2bf53271e20d

Comment 25

[Régis Caspar]

Thanks for the commit/push