607280 - In ContentAreaClick don't call getShortcutOrURI for link or panels clicks

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

Gavin said in https://bugzilla.mozilla.org/show_bug.cgi?id=549340#c43:

>+      let postData = {};
>+      let url = getShortcutOrURI(href, postData);
>+      if (!url)
>+        return true;
>+      loadURI(url, null, postData.value, false);

This is really messed up (existing behavior)... we shouldn't be calling
getShortcutOrURI for link clicks, even just for panels.

Comment 1

[Abhishek Potnis [:avp]]

Hi Gavin,
I am interested in working on this bug. I am a beginner and this would be my first bug, So could you please help me on getting started with this bug.

Comment 2

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

Sure thing. Do you already have a build environment and mercurial set up? The relevant code is all in browser/base/content/browser.js, and the patch is pretty simple: just remove the call to getShortcutOrURI in the "contentAreaClick" function, and instead pass "href" directly to loadURI().

Comment 3

[Abhishek Potnis [:avp]]

Yes I have downloaded the source code and built firefox. I checked the browser.js file
In the "contentAreaClick" function, the value returned by "getShortcutOrURI" is being assigned to "url" which is being checked in the next line. So if I replace the call to "getShortcutOrURI" with "LoadURI()" then shall I catch the value returned by it in "url" ? Also the "loadURI()" function is having 4 parameters, so the first  parameter in the call will be "href", what shall I pass as other three parameters ?

Comment 4

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

In comment 0, there are 5 quoted lines. Fixing this bug involves deleting the first four, and replacing "url" with "href" in the 5th.

Comment 5

[Abhishek Potnis [:avp]]

Attachment: This is the modified browser.js file

I have edited the browser.js and am attaching it here, but I donot know how to write a test for it, I built firefox after editing and ran it, it worked with no problems. I also dont understand the procedure for creating a patch. Please help me with this.

Thanks

Comment 6

[Abhishek Potnis [:avp]]

Sorry Gavin,
I did not know the procedure to create a patch then. I am creating a patch and uploading it now

Comment 7

[Abhishek Potnis [:avp]]

Attachment: I have edited the browser.js and am attaching it here, but I donot know how to test it, I built firefox after editing and ran it, it worked with no problems

Comment 8

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

Comment on attachment 638107 [details] [diff] [review]
I have edited the browser.js and am attaching it here, but I donot know how to test it, I built firefox after editing and ran it, it worked with no problems

You'll also need to replace "postData.value" with "null", since the postData variable no longer exists.

To test this manually, you need to open a page in a sidebar (per http://lifehacker.com/294684/load-any-site-in-your-firefox-sidebar), and the page must contain a link whose href matches e.g. a bookmark keyword. When clicking the link in the sidebar without this patch, the bookmark keyword will be activated and you will load the bookmarked page in the main content area. With this patch, clicking the link will fail to do anything.

To add an automated test, you should be able to add a test to browser_contentAreaClick.js.

Comment 9

[Abhishek Potnis [:avp]]

Attachment: Made the suggested changes in browser.js

Hi Gavin,

I have replaced "postData.value" with "null" as suggested by you. 

But I did not understand by what you meant by "the page must contain a link whose href matches e.g. a bookmark keyword." as in did you mean |position1| or |position2| in <a href= "position1">position2</a>. I tried this (both |position1| and |position2| containing the bookmark keyword) without the patch but the link present in |position1| is opening in the main content area and the keyword is not being activated. So could you please help me with this ? 

Also how shall I proceed with writing the tests for this bug ?
Please could you assign this bug to me as I have already started working on it.

Thanks.

Comment 10

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

Comment on attachment 643357 [details] [diff] [review]
Made the suggested changes in browser.js

This looks good, sorry for the unreasonable delay here :(

nit: you'll want to undo the addition of the trailing whitespace after that closing brace (that's why it shows up as changed in the diff)

Comment 11

[Abhishek Potnis [:avp]]

Attachment: Removed the call to getShortcutOrURI in the "contentAreaClick" function

Comment 12

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

Comment on attachment 665926 [details] [diff] [review]
Removed the call to getShortcutOrURI in the "contentAreaClick" function

This patch gets rid of the "url" variable but then continues to use it for the call to loadURI - that's not going to work.

Comment 13

[Abhishek Potnis [:avp]]

Attachment: Removed the call to getShortcutOrURI in the "contentAreaClick" function

Replaced the |url| in the previous patch with |href|

Comment 14

[Abhishek Potnis [:avp]]

Attachment: edited the test in browser_contentAreaClick.js

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a83ad99f062

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2a83ad99f062