Closed Bug 1120967 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- verified

People

(Reporter: adalucinet, Assigned: Fischer)

References

Details

Attachments

(3 files)

popup window.png
127.91 KB, image/png
Details
Bug 1120967 - Broken middle/right click on links via about:preferences pages;
58 bytes, text/x-review-board-request
jaws
: review+
Details
html_a_help_button_UI.png
678.82 KB, image/png
Details 
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.
@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)
Attached image 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: nobody → fliu
Flags: needinfo?(fliu)
(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.
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
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.
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-
@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
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)
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+

    
    

    
      
  

    
    

    
  
@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 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+

    
    

    
      
  

    
    

    
  
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
Closed: 8 years ago

          status-firefox51:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

    
    

    
  
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
QA Whiteboard: [testday-20160909]
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: