The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 19

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: mak, Assigned: avp)

Tracking

Trunk
Firefox 19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=gavin][lang=js])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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.
Summary: In ContentAreaClick don't call getShurtcutOrURI for link or panels clicks → In ContentAreaClick don't call getShortcutOrURI for link or panels clicks
Whiteboard: [good first bug][mentor=gavin][lang=js]
(Assignee)

Comment 1

5 years ago
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.
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().
(Assignee)

Comment 3

5 years ago
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 ?
In comment 0, there are 5 quoted lines. Fixing this bug involves deleting the first four, and replacing "url" with "href" in the 5th.
(Assignee)

Comment 5

5 years ago
Created attachment 638080 [details]
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
(Assignee)

Comment 6

5 years ago
Sorry Gavin,
I did not know the procedure to create a patch then. I am creating a patch and uploading it now
(Assignee)

Comment 7

5 years ago
Created 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
Attachment #638080 - Attachment is obsolete: true
Attachment #638107 - Flags: feedback?(gavin.sharp)
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.
Attachment #638107 - Flags: feedback?(gavin.sharp) → feedback-
(Assignee)

Comment 9

5 years ago
Created attachment 643357 [details] [diff] [review]
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.
Attachment #638107 - Attachment is obsolete: true
Attachment #643357 - Flags: feedback?(gavin.sharp)
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)
Attachment #643357 - Flags: feedback?(gavin.sharp) → feedback+

Updated

5 years ago
Assignee: nobody → abhishekp.bugzilla
Status: NEW → ASSIGNED
(Assignee)

Comment 11

5 years ago
Created attachment 665926 [details] [diff] [review]
Removed the call to getShortcutOrURI in the "contentAreaClick" function
Attachment #643357 - Attachment is obsolete: true
Attachment #665926 - Flags: feedback?(gavin.sharp)
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.
Attachment #665926 - Flags: feedback?(gavin.sharp) → feedback-
(Assignee)

Comment 13

5 years ago
Created attachment 675102 [details] [diff] [review]
Removed the call to getShortcutOrURI in the "contentAreaClick" function

Replaced the |url| in the previous patch with |href|
Attachment #665926 - Attachment is obsolete: true
Attachment #675102 - Flags: feedback?(gavin.sharp)
Attachment #675102 - Flags: feedback?(gavin.sharp) → feedback+
Attachment #675102 - Flags: feedback+ → review+
(Assignee)

Comment 14

4 years ago
Created attachment 678109 [details] [diff] [review]
edited the test in browser_contentAreaClick.js
Attachment #675102 - Attachment is obsolete: true
Attachment #678109 - Flags: review?(gavin.sharp)
Attachment #678109 - Flags: review?(gavin.sharp) → review+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a83ad99f062
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2a83ad99f062
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.