Closed Bug 383880 Opened 16 years ago Closed 16 years ago

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

Categories

(Firefox :: Page Info Window, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3 alpha8

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: polish)

Attachments

(1 file, 5 obsolete files)

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.
Flags: blocking-firefox3?
Attached patch Make feed links clickable (obsolete) — Splinter Review
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.
Attachment #267851 - Flags: ui-review?
Attachment #267851 - Flags: review?(mano)
Attachment #267851 - Flags: ui-review? → ui-review?(beltzner)
Comment on attachment 267851 [details] [diff] [review]
Make feed links clickable

The text-link binding should be used for exposing links in the UI.
Attachment #267851 - Flags: review?(mano) → review-
Updated patch using the text-link binding.
Attachment #267851 - Attachment is obsolete: true
Attachment #268017 - Flags: ui-review?(beltzner)
Attachment #268017 - Flags: review?(mano)
Attachment #267851 - Flags: ui-review?(beltzner)
So what's this place-holder for given that it is hidden?
(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.
Wouldn't a simple <xul:spacer flex="1"/> work just as well?
(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).
Attachment #268017 - Attachment is obsolete: true
Attachment #268919 - Flags: ui-review?(beltzner)
Attachment #268919 - Flags: review?(mano)
Attachment #268017 - Flags: ui-review?(beltzner)
Attachment #268017 - Flags: review?(mano)
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?
(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()?
Updated to apply cleanly on trunk.  No other changes made.
Attachment #268919 - Attachment is obsolete: true
Attachment #269633 - Flags: ui-review?(beltzner)
Attachment #269633 - Flags: review?(mano)
Attachment #268919 - Flags: ui-review?(beltzner)
Attachment #268919 - Flags: review?(mano)
(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.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3] polish
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.
Attachment #269633 - Flags: review?(mano) → review-
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).
Attachment #269633 - Attachment is obsolete: true
Attachment #271412 - Flags: review?(mano)
Attachment #269633 - Flags: ui-review?(beltzner)
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.
Attachment #271412 - Flags: review?(mano) → review-
Retargetting to the next milestone.
Target Milestone: Firefox 3 alpha6 → Firefox 3 M7
Revised to address the issues in comment 14.
Attachment #271412 - Attachment is obsolete: true
Attachment #273911 - Flags: review?(mano)
Comment on attachment 273911 [details] [diff] [review]
Make feed links clickable (revised again to use openUILink)

r=mano.
Attachment #273911 - Flags: review?(mano) → review+
Keywords: checkin-needed
Whiteboard: [wanted-firefox3] polish → [wanted-firefox3] polish [checkin needed]
Whiteboard: [wanted-firefox3] polish [checkin needed] → [wanted-firefox3] polish
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Keywords: verifyme
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3] polish → polish
Keywords: polish
Whiteboard: polish
Depends on: 418879
Flags: in-litmus?
Keywords: verifyme
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.