The default bug view has changed. See this FAQ.

Open "Links" URLs in browser

RESOLVED FIXED

Status

SeaMonkey
Page Info
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Karsten Düsterloh, Assigned: ewong)

Tracking

Trunk
Other
Other

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

6 years ago
Page Info's "Links" tabs shows all links of a page, but you can't anything with them except copying. 

The context menu should have the usual options to open in a new window/tab etc., and (double/middle)clicking should work as usual as well.
(Assignee)

Comment 1

5 years ago
Created attachment 621293 [details] [diff] [review]
Open 'Links' URLs in browser.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #621293 - Flags: review?(neil)

Comment 2

5 years ago
Comment on attachment 621293 [details] [diff] [review]
Open 'Links' URLs in browser.

>+function getList()
You should move this function earlier and make doCopy call it too.
I'd like to think there was a better name for this but I can't think of one.

>+function onOpenInTabClick()
>+{
>+  var browser = document.getElementById("content");
Unnecessary.

>+  var linkList = getList();
>+
>+  if (linkList)
This is always true, because getList() returns an array object. Maybe you meant to check the length?

>+    while (tmp = linkList.pop())
>+      openNewTabWith(tmp, gDocument);
This probably opens the tabs in the wrong order. Also it will fail as soon as you get to an insecure link. Calling openUILinkArrayIn would be better except that it does no security checks at all. (This could be fixed by adding some extra security checks somewhere. You can choose what you want to do if only some links fail the security check.)

>+    while (tmp = linkList.pop())
>+      openNewWindowWith(tmp, this.ownerDocument);
Opening a separate window for each link doesn't seem to make sense.

>   <menupopup id="picontext">
>+  	<menuitem id="menu_openInTab"
>+  		        label="&openInTab.label;"
>+  		        command="cmd_openInTab"
>+  		        accesskey="&openInTab.accesskey;"/>
Nit: wrong indentation.
Attachment #621293 - Flags: review?(neil) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 621979 [details] [diff] [review]
Open 'Links' URLs in browser. (v2)
Attachment #621293 - Attachment is obsolete: true
Attachment #621979 - Flags: review?(neil)

Comment 4

5 years ago
Comment on attachment 621979 [details] [diff] [review]
Open 'Links' URLs in browser. (v2)

Looks good from a quick skim, haven't tried it out yet. I see you added the security check when getting the list of links. This means that if the selected link is insecure, then nothing might happen. I guess we should follow up to disable the menuitems in this case. (I did wonder about combining the link opening functions into a single function and moving the security check there, but even then you run into the problem of having an insecure link selected.)

>+function getList()
Nit: How about calling this getSelectedLinks?

>+  if (linkList.length > 0)
Nit: don't need to compare > 0, if (linkList.length) should suffice. (x2)

>+  {
>+    openUILinkArrayIn(linkList, 'tab');
>+  }
Nit: file style is not to use {}s for a single line. (x2)

>+    openUILinkArrayIn(linkList, 'window');
[If the functions hadn't been so small, I would have suggested combining them into one function with a parameter which we forward to openUILinkArrayIn.]
(Assignee)

Comment 5

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #4)
> 
> >+    openUILinkArrayIn(linkList, 'window');
> [If the functions hadn't been so small, I would have suggested combining
> them into one function with a parameter which we forward to
> openUILinkArrayIn.]

Both onOpenInWindowClick() and onOpenInTabClick() have pretty much the
same content, just a different parameter for openUILinkArrayIn().  Would
it be actually better to just do a function like:

  function onOpenIn(mode)
  {
    var linkList = getSelectedLinks();

    if (linkList.length)
      openUILinkArrayIn(linkList, mode);
  }

then just call the function in the xul file and use parameters like you mentioned?

Comment 6

5 years ago
(In reply to Edmund Wong from comment #5)
> Both onOpenInWindowClick() and onOpenInTabClick() have pretty much the
> same content
Given that they have three lines of code each, then you're calling two identical lines "pretty much the same" ;-)

Comment 7

5 years ago
Comment on attachment 621979 [details] [diff] [review]
Open 'Links' URLs in browser. (v2)

>+<!ENTITY  openInTab.label        "Open in Tab">
>+<!ENTITY  openInTab.accesskey    "T">
>+<!ENTITY  openInWindow.label     "Open in Window">
>+<!ENTITY  openInWindow.accesskey "W">
These should say New Tab/Window. r=me with this and my other nits fixed.
Attachment #621979 - Flags: review?(neil) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 623385 [details] [diff] [review]
Open 'Links' URLs in browser (v3)

requesting review on the js code.
Attachment #621979 - Attachment is obsolete: true
Attachment #623385 - Flags: review?(neil)

Comment 9

5 years ago
Comment on attachment 623385 [details] [diff] [review]
Open 'Links' URLs in browser (v3)

I've just discovered a major problem with these patches...

All the trees currently use the same context menu, so you see the extra menuitems on e.g. the Meta and Fields trees where they make no sense.

Worse, it breaks the "copy" command on those trees, because you added a security check, but we're not copying links...
Attachment #623385 - Flags: review?(neil) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 623471 [details] [diff] [review]
Open 'Links' URLs in browser. (v4)
Attachment #623385 - Attachment is obsolete: true
Attachment #623471 - Flags: review?(neil)
Comment on attachment 623471 [details] [diff] [review]
Open 'Links' URLs in browser. (v4)

>   <menupopup id="picontext">
>-    <menuitem id="menu_selectall" label="&selectall.label;" command="cmd_selectall" accesskey="&selectall.accesskey;"/>
>-    <menuitem id="menu_copy"      label="&copy.label;"      command="cmd_copy"      accesskey="&copy.accesskey;"/>
>+    <menuitem id="menu_selectall"
>+              label="&selectall.label;"
>+              command="cmd_selectall"
>+              accesskey="&selectall.accesskey;"/>
>+    <menuitem id="menu_copy"
>+              label="&copy.label;"
>+              command="cmd_copy"
>+              accesskey="&copy.accesskey;"/>
>+  </menupopup>
You don't need this change any more.

>+    <menuitem id="menu_selectall"
...
>+    <menuitem id="menu_copy"
Duplicate ids. r=me with that fixed.

>+              label="&copy.label;"
>+              command="cmd_copyLinks"
Hmm, I wonder whether we should give this menuitem a different label...
Attachment #623471 - Flags: review?(neil) → review+
(Assignee)

Comment 12

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #11)
> Comment on attachment 623471 [details] [diff] [review]
> Open 'Links' URLs in browser. (v4)
> 
> >   <menupopup id="picontext">
> >-    <menuitem id="menu_selectall" label="&selectall.label;" command="cmd_selectall" accesskey="&selectall.accesskey;"/>
> >-    <menuitem id="menu_copy"      label="&copy.label;"      command="cmd_copy"      accesskey="&copy.accesskey;"/>
> >+    <menuitem id="menu_selectall"
> >+              label="&selectall.label;"
> >+              command="cmd_selectall"
> >+              accesskey="&selectall.accesskey;"/>
> >+    <menuitem id="menu_copy"
> >+              label="&copy.label;"
> >+              command="cmd_copy"
> >+              accesskey="&copy.accesskey;"/>
> >+  </menupopup>
> You don't need this change any more.

Reverted.

> 
> >+    <menuitem id="menu_selectall"
> ...
> >+    <menuitem id="menu_copy"
> Duplicate ids. r=me with that fixed.
> 
> >+              label="&copy.label;"
> >+              command="cmd_copyLinks"
> Hmm, I wonder whether we should give this menuitem a different label...

Fixed with some additional code to determine whether the user selected
one link or more than one link and would change the popup menu item
to use the plural version.   So re-requesting review on this change.
(Assignee)

Comment 13

5 years ago
Created attachment 623564 [details] [diff] [review]
Open 'Links' URLs in browser (v5)
Attachment #623471 - Attachment is obsolete: true
Attachment #623564 - Flags: review?(neil)
(Assignee)

Comment 14

5 years ago
Created attachment 623567 [details] [diff] [review]
Open 'Links' URLs in browser (v5)
Attachment #623564 - Attachment is obsolete: true
Attachment #623567 - Flags: review?(neil)
Attachment #623564 - Flags: review?(neil)
Comment on attachment 623567 [details] [diff] [review]
Open 'Links' URLs in browser (v5)

>+function changeLinksLabel()
>+{
>+	var linkLabel = document.getElementById("menu_copy_links");
>+	var itemList = getSelectedItems(false);
>+
>+  if (itemList.length > 1)
>+    linkLabel.label = gBundle.getString("linkCopyLinks");
This isn't the correct way to do plurals (you should use PluralForm.jsm) but it just goes to show that my comment isn't as throwaway as I'd hoped. Let's just forget that I ever mentioned it, and go back to the original label.

>+    <menuitem id="menu_selectall"
Still duplicates an existing id.

And of course there's the tabs ;-)
Attachment #623567 - Flags: review?(neil) → review-
(Assignee)

Comment 16

5 years ago
Created attachment 624297 [details] [diff] [review]
Open 'Links' URLs in browser (v6)
Attachment #623567 - Attachment is obsolete: true
Attachment #624297 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Attachment #624297 - Flags: review?(neil)
(Assignee)

Comment 17

5 years ago
Created attachment 624298 [details] [diff] [review]
Open 'Links' URLs in browser (v6)
Attachment #624297 - Attachment is obsolete: true
Attachment #624298 - Flags: review?(neil)
Comment on attachment 624298 [details] [diff] [review]
Open 'Links' URLs in browser (v6)

>+    <menuitem id="menu_copy_links"
>+              label="&copyLinks.label;"
>+              command="cmd_copyLinks"
Nit: could use id=menu_copyLinks to match cmd_copyLinks perhaps?

>+<!ENTITY  copyLinks.key             "o">
Nit: unused. r=me with this removed.

>+<!ENTITY  copyLinks.label           "Copy Link">
Maybe we should use "Copy Link(s)" here. (I've done it before, and as yet, none of the localisers have screamed at me...)

>+<!ENTITY  copyLinks.accesskey       "o">
Nit: Wouldn't "L" or maybe "C" be better?
Attachment #624298 - Flags: review?(neil) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 625483 [details] [diff] [review]
Open 'Links' URLs in browser. (v7)
Attachment #624298 - Attachment is obsolete: true
Attachment #625483 - Flags: review+
(Assignee)

Comment 20

5 years ago
Pushed to comm-central:

http://hg.mozilla.org/comm-central/rev/26efbc349918
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.