383880 - The Page Info RSS section's feed links should be clickable

Status: VERIFIED FIXED

(Firefox :: Page Info Window, enhancement)

Description

[(no longer active)]

The feed links in the Page Info dialog's RSS section should be clickable, so that upon clicking any of them, the corresponding feed is loaded in the browser.  This can be useful e.g. when a user tries to preview a feed before subscribing to it, for example.

I'll be posting a patch to support this shortly.

Comment 1

[(no longer active)]

Attachment: Make feed links clickable

This patch makes the feed links in the Page Info section clickable.  Upon clicking, the user will visit the feed URL.

The implementation tries to keep the existing UI intact, and only add hyperlink functionality to it.  The link appearance is achieved via CSS.  Also, the textbox containing the link is properly resized to the appropriate width to hold the link, and does not stretch more than that (because in that case, clicking on the right side of the link (which is also inside the XUL textbox element) would trigger the click action.  Also, pressing Enter on the keyboard activates the link too.

All the existing functionality of the text box has been preserved.  For example, one could still right click, Select All, and copy the link to the clipboard.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 267851 [details] [diff] [review]
Make feed links clickable

The text-link binding should be used for exposing links in the UI.

Comment 3

[(no longer active)]

Attachment: Make feed links clickable (revised)

Updated patch using the text-link binding.

Comment 4

[Asaf Romano (gone)]

So what's this place-holder for given that it is hidden?

Comment 5

[(no longer active)]

(In reply to comment #4)
> So what's this place-holder for given that it is hidden?

Without this place-holder, the link element would stretch to the whole width of the list item.  The user clicks on a list item to expand it (and, for example, see the Subscribe button on the list item).  Without this placeholder, if the user clicks on the right side of the link element to activate the list item, the link would also become activated, which may not be what the user intended (because the user did not click on a link, as far as the visual representation of the link shows).

Since a XUL label that's not a link does not stretch to the whole width of its container, I'm using it in a vbox to make the box with equal to exactly what is needed to display the URI text, and make it hidden so that only the link is visible.  This way, the link stretches only to the width of the vbox, which is equal to the width of the link's text.  Therefore, the link is activated *only* when the user clicks on the link's text, and not when she clicks on the right side of it.

If there is an easier way to achieve the same effect, I'm all for it, but this was the simplest solution I could find.

Comment 6

[Asaf Romano (gone)]

Wouldn't a simple <xul:spacer flex="1"/> work just as well?

Comment 7

[(no longer active)]

Attachment: Make feed links clickable (revised again)

(In reply to comment #6)
> Wouldn't a simple <xul:spacer flex="1"/> work just as well?

Yes, it sure does!  Sorry for my inarticulate XUL!  :-)

Updated the patch accordingly.

In addition, the new patch respects the tabbed browsing preferences (see bug 262575 for reference).

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 268919 [details] [diff] [review]
Make feed links clickable (revised again)

>Index: browser/base/content/pageinfo/feeds.js
>===================================================================

>+function openURL(aURL)

Is it possible to use openUILink from utilityOverlay.js?

Comment 9

[(no longer active)]

(In reply to comment #8)
> >+function openURL(aURL)
> 
> Is it possible to use openUILink from utilityOverlay.js?
> 

Hmmm.  openUILink() does not handle the browser.link.open_newwindow pref at all, even though the processing it performs is interesting.  I looked through browser.js and saw openUILink()/openUILinkIn() used in several places.  At one place <http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3148>, the browser.link.open_newwindow pref is respected.  At other places it's not.

I'm not sure what the correct behavior is here.  I have based my code on the "Get Extensions" link in the Add-on manager window.  If using openUILink() is the way to go, I'll be happy to use it.  Maybe we need to file a new bug and add the code to handle the browser.link.open_newwindow pref in whereToOpenLink()?

Comment 10

[(no longer active)]

Attachment: Make feed links clickable (revised again to apply cleanly on trunk)

Updated to apply cleanly on trunk.  No other changes made.

Comment 11

[(no longer active)]

(In reply to comment #9)
> I'm not sure what the correct behavior is here.  I have based my code on the
> "Get Extensions" link in the Add-on manager window.  If using openUILink() is
> the way to go, I'll be happy to use it.  Maybe we need to file a new bug and
> add the code to handle the browser.link.open_newwindow pref in
> whereToOpenLink()?
> 

Perhaps we can have beltzner's input on how the opening of a link should be handled here as well.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 269633 [details] [diff] [review]
Make feed links clickable (revised again to apply cleanly on trunk)

Why use the opener? If pageInfo.xul doesn't load utilityOverlay.js, just make it do so.

Comment 13

[(no longer active)]

Attachment: Make feed links clickable (revised to use the function in utilityOverlay.js)

Revised the code to use the function defined in utilityOverlay.js, and removed the check for its existence (because pageInfo.xul does include utilityOverlay.js).

Comment 14

[Asaf Romano (gone)]

Comment on attachment 271412 [details] [diff] [review]
Make feed links clickable (revised to use the function in utilityOverlay.js)

openUILink should be used here (make sure to pass the event as well, for modifiers/mouse gestures support).

browser.link.open_newwindow isn't supported by chrome.

Comment 15

[(no longer active)]

Retargetting to the next milestone.

Comment 16

[(no longer active)]

Attachment: Make feed links clickable (revised again to use openUILink)

Revised to address the issues in comment 14.

Comment 17

[Asaf Romano (gone)]

Comment on attachment 273911 [details] [diff] [review]
Make feed links clickable (revised again to use openUILink)

r=mano.

Comment 18

[Michael Wu [:mwu]]

Checking in browser/base/content/pageinfo/feeds.xml;
/cvsroot/mozilla/browser/base/content/pageinfo/feeds.xml,v  <--  feeds.xml
new revision: 1.3; previous revision: 1.2
done

Comment 19

[Tanner M. Young [:tmyoung]]

Verified Fixed.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091205 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091205042811