Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Broken middle/right click on links via about:preferences pages

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Preferences
VERIFIED FIXED
3 years ago
7 months ago

People

(Reporter: adalucinet, Assigned: Fischer)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Reproducible on 36 beta 1 (Build ID: 20150112201205), Aurora 37.0a2 (Build ID: 20150113004007) and Nightly 38.0a1 (Build ID: 20150113030205)
Platforms: Windows 7 32-bit, Windows 8.1 64-bit, Mac OSX 10.9.5 and Ubuntu 14.04 32-bit

Steps to reproduce:
1. Launch Firefox.
2. Go to about:preferences
3. Middle/right click on links via available pages - Eg:
-- Go to about:preferences#privacy and right/middle click on 'Learn more', 'clear your recent history' and 'remove individual cookies'.

Expected result: Links are opened in new tabs (same behavior as in about:preferences#sync).
Actual result: Nothing happens and 'uncaught exception: Load of # denied.' message is displayed in Browser console when middle click on 'clear your recent history' and 'remove individual cookies'.

Notes:
1. Not a regression - reproducible with Nightly 2014-05-19.
2. In about:preferences#privacy, a context menu is displayed for 'clear your recent history' and 'remove individual cookies'; when 'Bookmark this link' is selected: 'NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService2.newURI]' message is thrown by BrowserUtils.jsm:70:0 in Browse Console.
(Assignee)

Comment 1

a year ago
@Jack & Tina,
I guess we need to think about one unified behavior of middle/right click on links.
Please have a look, thanks
Flags: needinfo?(thsieh)
Flags: needinfo?(jalin)

Comment 2

a year ago
Created attachment 8775451 [details]
popup window.png

Hi Fischer,

We would like to make browser experience consistent, so

About right click,

1.For opening a website link, such as 
    'Learn more', 
    'Question mark', 
    'Search-Add more search engines...', 
    'Privacy-Change preferences for search engine suggestions...', 
    'Sync-Manage Account', 
    'Sync-Android/iOS', 
    'Sync-Terms of Service/Privacy Notice', 
can show a popup window like the attachment?

2.For launching an action, such as 'clear your recent history' and 'remove individual cookies', please keep current design.


About middle click,

Is it possible to make middle click same as left click? For example,

1. When the user presses middle click on a website link 'Learn more', open the website on a new tab. 

2. When the user presses middle click on a link 'clear your recent history', show 'Clear Recent History' popup window.

3. When the user presses middle click on a button 'Make Default', show its popup window.


Thank you
Flags: needinfo?(jalin) → needinfo?(fliu)
Flags: needinfo?(thsieh)
(Assignee)

Updated

a year ago
Assignee: nobody → fliu
Flags: needinfo?(fliu)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

11 months ago
(In reply to Jack Lin[:jacklin] from comment #2)
> Created attachment 8775451 [details]
> popup window.png
> 
> Hi Fischer,
> 
> We would like to make browser experience consistent, so
> 
> About right click,
> 
> 1.For opening a website link, such as 
>     'Learn more', 
>     'Question mark', 
>     'Search-Add more search engines...', 
>     'Privacy-Change preferences for search engine suggestions...', 
>     'Sync-Manage Account', 
>     'Sync-Android/iOS', 
>     'Sync-Terms of Service/Privacy Notice', 
> can show a popup window like the attachment?
> 
> 2.For launching an action, such as 'clear your recent history' and 'remove
> individual cookies', please keep current design.
> 
> 
> About middle click,
> 
> Is it possible to make middle click same as left click? For example,
> 
> 1. When the user presses middle click on a website link 'Learn more', open
> the website on a new tab. 
> 
> 2. When the user presses middle click on a link 'clear your recent history',
> show 'Clear Recent History' popup window.
> 
> 3. When the user presses middle click on a button 'Make Default', show its
> popup window.
> 
> 
> Thank you


After discussion and demo with the UX :jalin, the unified middle/right mouse click behaviors are:

* For right-mouse clicking a website link, such as 
     'Learn more', 
     'Question mark'(The "?" button on the top-right side), 
     'Search-Add more search engines...', 
     'Sync-Manage Account',
     'Sync-Android/iOS', 
     'Sync-Terms of Service/Privacy Notice', 
will show a menu like the attachment of "popup window.png"

* For middle-mouse clicking a website link will open that link in a new tab

* For middle/right-mouse clicking
     'Privacy - Change preferences for search engine suggestions...', 
     'Privacy - clear your recent history',
     'Privacy - show 'Clear Recent History',
     'Privacy - Manage your Do Not Track settings"
will launch the corresponding function.
(Assignee)

Comment 7

11 months ago
Hi Jared,

This patch implements the unified middle/right mouse click behaviors based on Comment 6.
What it does is simply to put all website links inside html:a elements. As a result, the nsContextMenu at [1] would recognize and open the menu when right-mouse clicking.
In order to achieve this, this patch also modifies some utility JS functions and CSS rules.

As for the middle/right-mouse clicking parts, the patch basically do nothing about it since the default behavior of middle-mouse clicking on html:a would open that link and the default behavior on middle/right-mouse clicking on, like 'Privacy - clear your recent history', would trigger the corresponding function.

The try is at [2].

Thank you

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#614
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d00977a868c
(Assignee)

Comment 8

11 months ago
mozreview-review
Comment on attachment 8781045 [details]
Bug 1120967 - Broken middle/right click on links via about:preferences pages;

https://reviewboard.mozilla.org/r/71560/#review69414

::: browser/components/preferences/in-content/privacy.xul:284
(Diff revision 3)
>              preference="browser.urlbar.suggest.bookmark"/>
>    <checkbox id="openpageSuggestion" label="&locbar.openpage.label;"
>              accesskey="&locbar.openpage.accesskey;"
>              preference="browser.urlbar.suggest.openpage"/>
> -  <label class="text-link" onclick="if (event.button == 0) gotoPref('search')">
> +  <label class="text-link" onclick="gotoPref('search')">
>      &suggestionSettings.label;

Remove the condition of if (event.button == 0) so middle/right mouse click would also work.
Duplicate of this bug: 1273515

Comment 10

11 months ago
mozreview-review
Comment on attachment 8781045 [details]
Bug 1120967 - Broken middle/right click on links via about:preferences pages;

https://reviewboard.mozilla.org/r/71560/#review69906

Bug 1268943 should have made it so that you didn't need to make most of these changes. Anything that is a xul:label with class="text-link" should work.
Attachment #8781045 - Flags: review?(jaws) → review-
(Assignee)

Comment 11

11 months ago
@Jared,

Bug 1268943 does not cover all of this bug.
Like Comment 6, this patch enables context menu on external links when right-mouse clicking. 

Because the namespace of <label class="text-link"> is XUL namespace, context menu is not allowed to be displayed on it when calling nsContextMenu#setTarget at [1].
This patch moves external links from <label class="text-link"> to <html:a> so as to enable context menu.

This change is quite reasonable because those external "HTTP" links should have context menu just like other links on web pages. Users would have consistent UX on links.

And thanks for the works in Bug 1268943. This patch basically doesn't have to handle middle-mouse click.

Maybe we should change this bug title to "No context menu when right clicking on external links inside about:preferences page".

How do you think about this UX improvement?

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#627
Flags: needinfo?(jaws)
We can relax the check in nsContextMenu since it was only added for right-clicking on buttons. If it's a label with class="text-link" we could let it through, though some code may have to be refactored. This would also fix the same issue with about:addons when you right click on the author's name of an addon. 

If we allow right-clicking we should also allow middle-clicking.

I don't see much gain in changing the help buttons to html:a, as they already open in a new tab. I'm not sure what other actions people would do with these buttons.
Flags: needinfo?(jaws)
I forgot to link it but it was bug 404536 which added the check for !XUL in nsContextMenu.js
Blocks: 1028029
Comment hidden (mozreview-request)
(Assignee)

Comment 15

11 months ago
Comment on attachment 8781045 [details]
Bug 1120967 - Broken middle/right click on links via about:preferences pages;

Change r? to f?
-------------------------------
Hi Jared,

Try the relaxing <label class="text-link"> approach. Please notice that even relaxing <label class="text-link">, still need to modify some JS utility methods in order to set href attribute.

> If we allow right-clicking we should also allow middle-clicking.
- Currently middle-clicking on <label class="text-link"> already opens new tab so what else should be take cared ?

> I don't see much gain in changing the help buttons to html:a, as they already open in a new tab.
- From the UX's point, the right-click context menu provides user more options, such as "Open Link in New Window", "Copy Link Location". These options are what user may want to act on a link so that would be good to have it.
To change that "?" link, there are 2 approaches. One is change to <label class="text-link">, another is <html:a>. Do we have any concern or I am thinking that using web standard <html:a> may be better.

Thank you
Attachment #8781045 - Flags: review?(jaws) → feedback?(jaws)
Duplicate of this bug: 1297170

Comment 17

11 months ago
mozreview-review
Comment on attachment 8781045 [details]
Bug 1120967 - Broken middle/right click on links via about:preferences pages;

https://reviewboard.mozilla.org/r/71560/#review72992

This looks good (feedback+ from me).

We should add a test case the changes to nsContextMenu.js.

I'm OK with changing the Help button to be an html:a link as long as we can keep the styling, keyboard access, and focus-ring behavior unchanged.

::: browser/base/content/nsContextMenu.js:821
(Diff revision 4)
> -             ((elem instanceof HTMLAnchorElement && elem.href) ||
> +             (isXULTextLinkLabel(elem) ||
> +              (elem instanceof HTMLAnchorElement && elem.href) ||

browser_contextMenu.js needs to have a test case added.
Attachment #8781045 - Flags: feedback?(jaws) → feedback+
(Assignee)

Comment 18

11 months ago
Created attachment 8788028 [details]
html_a_help_button_UI.png
Comment hidden (mozreview-request)
(Assignee)

Comment 20

11 months ago
@Jared,

The help buttons are updated to <html:a class="help-button>. The UI styling is kept, please see the html_a_help_button_UI.png.

The test case for xul text-link label has been added.

The local test is passed and the try is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a97085618b8c

Please have a look.

Thank you.

Comment 21

11 months ago
mozreview-review
Comment on attachment 8781045 [details]
Bug 1120967 - Broken middle/right click on links via about:preferences pages;

https://reviewboard.mozilla.org/r/71560/#review75208

::: browser/components/preferences/in-content/preferences.js:85
(Diff revision 5)
> +  for (let i of items) {
> +    // Extract category from id. For instance, extract "sync" from "category-sync".
> +    id = i.getAttribute("id");
> +    category = id.substr(id.indexOf("-") + 1);
> +    helpBtn = document.querySelector("#header-" + category + " > .help-button");
> +    helpBtn.setAttribute("href", getHelpLinkURL(i.getAttribute("helpTopic")));
> +  }

We already have code that will get the category name from the richlistitem's value. Also, please use `categories` instead of `items` and `category` instead of `i`. Also, please use template strings.

for (let category of categories) {
  let value = category.value;
  let name = internalPrefCategoryNameToFriendlyName(value);
  let helpSelector = `#header-${name} > .help-button`;
  let helpButton = document.querySelector(helpSelector);
  helpButton.setAttribute("href", getHelpLinkURL(category.getAttribute("helpTopic")));
}

::: toolkit/themes/shared/in-content/common.inc.css:316
(Diff revision 5)
> +
> +html|*.help-button:hover {
> +  background-image: url("chrome://global/skin/in-content/help-glyph.svg#help-hover");
> +}
> +
> +html|*.help-button:active {

html|*.help-button:hover:active {
  ...
}
Attachment #8781045 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 23

11 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a639195ac0e5
Keywords: checkin-needed

Comment 24

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/7ec0c9ca41df
Broken middle/right click on links via about:preferences pages. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ec0c9ca41df
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Comment 26

11 months ago
I have reproduced this on firefox nightly according to(2015-01-13)

Fixing bug is verified on Latest Nightly-- Build ID:(20160908030434 ), User Agent: Mozilla/5.0 (Windows NT 10.0; rv:51.0) Gecko/20100101 Firefox/51.0

Those issues are solved.It seems ok on latest Nightly. :-)

Tested OS--Windows10 32bit

Updated

11 months ago
QA Whiteboard: [testday-20160909]

Updated

11 months ago
Depends on: 1301813
I have reproduce this bug with Nightly 38.0a1 (2015-01-13) on Ubuntu 16.04 LTS !

This bug's fix is verified with latest Beta !

Build Id   :   20161219140923
User Agent :   Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
[testday-20170106]
Thanks!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.