Closed
Bug 383880
Opened 18 years ago
Closed 17 years ago
The Page Info RSS section's feed links should be clickable
Categories
(Firefox :: Page Info Window, enhancement)
Firefox
Page Info Window
Tracking
()
VERIFIED
FIXED
Firefox 3 alpha8
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
(Keywords: polish)
Attachments
(1 file, 5 obsolete files)
1.30 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•18 years ago
|
||
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)
Assignee | ||
Updated•18 years ago
|
Attachment #267851 -
Flags: ui-review? → ui-review?(beltzner)
Comment 2•18 years ago
|
||
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-
Assignee | ||
Comment 3•18 years ago
|
||
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)
Comment 4•18 years ago
|
||
So what's this place-holder for given that it is hidden?
Assignee | ||
Comment 5•18 years ago
|
||
(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•18 years ago
|
||
Wouldn't a simple <xul:spacer flex="1"/> work just as well?
Assignee | ||
Comment 7•18 years ago
|
||
(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 8•18 years ago
|
||
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?
Assignee | ||
Comment 9•18 years ago
|
||
(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()?
Assignee | ||
Comment 10•18 years ago
|
||
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)
Assignee | ||
Comment 11•18 years ago
|
||
(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.
Updated•18 years ago
|
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3] polish
Comment 12•18 years ago
|
||
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-
Assignee | ||
Comment 13•18 years ago
|
||
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 14•18 years ago
|
||
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-
Assignee | ||
Comment 15•18 years ago
|
||
Retargetting to the next milestone.
Target Milestone: Firefox 3 alpha6 → Firefox 3 M7
Assignee | ||
Comment 16•18 years ago
|
||
Revised to address the issues in comment 14.
Attachment #271412 -
Attachment is obsolete: true
Attachment #273911 -
Flags: review?(mano)
Comment 17•17 years ago
|
||
Comment on attachment 273911 [details] [diff] [review]
Make feed links clickable (revised again to use openUILink)
r=mano.
Attachment #273911 -
Flags: review?(mano) → review+
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Whiteboard: [wanted-firefox3] polish → [wanted-firefox3] polish [checkin needed]
Updated•17 years ago
|
Whiteboard: [wanted-firefox3] polish [checkin needed] → [wanted-firefox3] polish
Comment 18•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Updated•17 years ago
|
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3] polish → polish
Comment 19•15 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Flags: in-litmus?
You need to log in
before you can comment on or make changes to this bug.
Description
•