[ReadingList] Add section to bookmarks popup for reading list items and actions

VERIFIED FIXED in Firefox 39

Status

Firefox Graveyard
Reading List
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: clarkbw, Assigned: florian)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
Firefox 39
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

In the bookmarks popup launched from the toolbar lets add a section for the reading list that shows recent reading list items (limit?) and an action to open the sidebar.

I'm uncertain about the 'Clear the reading list' button, I'll let mmaslanely chime in on that action.

Design Mockup:
https://cdn.invisionapp.com/cdn/a26587b2bad82ff711f3abdc9b3160ac/screens/56354754/16/42g13AvYjfas6707BrY8MdHmnKAUY5WIo1BC0MqN0YOQ5u73rRUq8tdNQAjhvAJxoRlM9jhKgrgarpGgV6ATEQ7/Readlinglist-toolbar-menu-2x.png

Updated

3 years ago
Flags: qe-verify?
Flags: firefox-backlog+

Updated

3 years ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
Points: --- → 8

Updated

3 years ago
Blocks: 1132074
Bryan, is this only about adding a section in the menu that opens from the toolbar button, or do we also need it to be accessible from the "Bookmarks" menu of the menubar?

Also, is the icon available somewhere? I see there's a reasonably good looking icon on this mockup http://cl.ly/image/2x2A163C3b1D/o but I don't know where the icon file is.
Flags: needinfo?(clarkbw)
(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> Also, is the icon available somewhere? I see there's a reasonably good
> looking icon on this mockup http://cl.ly/image/2x2A163C3b1D/o but I don't
> know where the icon file is.

Icons in attachment 8562215 [details] (from bug 1131457).
(Reporter)

Comment 3

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> Bryan, is this only about adding a section in the menu that opens from the
> toolbar button, or do we also need it to be accessible from the "Bookmarks"
> menu of the menubar?

I know it is about the toolbar button, I'm not sure about the menubar.  @mmaslaney is the UX point person to answer that question.
Flags: needinfo?(clarkbw) → needinfo?(mmaslaney)

Updated

3 years ago
Flags: needinfo?(mmaslaney)
Florian, considering the amount of users that actually have the "Show Bookmarks" button in their menu, and the amount of work involved in the implementation, I would suggest holding off for now.
Created attachment 8564264 [details] [diff] [review]
WIP1

Mostly there, but a few things are missing:
- clicking an item doesn't do anything. I guess it's supposed to open the item in reader mode?
- I need to hide these new menus when the feature is pref'ed off.
- The readinglist items don't show in the native mac menu (when accessing this from the "Bookmarks" menu of the menubar). I guess this is because ReadingList.getItems() is async, and native menus don't get updated after the popupshowing event handler has returned. Does this really need to be asynchronous?
- I can't display the SVG icon in the native mac menu item. I think we'll need to have PNG versions if we care about this (currently the native mac menuitem just uses the generic bookmark icon).
Attachment #8564264 - Flags: feedback?(bmcbride)
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> - clicking an item doesn't do anything. I guess it's supposed to open the
> item in reader mode?

Nope, current plan for v1 is to just open the URL. We'll explore deeper ReaderMode integration in v2.

> - I need to hide these new menus when the feature is pref'ed off.

browser.readinglist.enabled is the pref. Bug 1123517 adds a ReadingListUI object to browser.js, which has an .enabled property.

> - I can't display the SVG icon in the native mac menu item. I think we'll
> need to have PNG versions if we care about this (currently the native mac
> menuitem just uses the generic bookmark icon).

Bah. Ok.
Drew, FYI:

> - The readinglist items don't show in the native mac menu (when accessing
> this from the "Bookmarks" menu of the menubar). I guess this is because
> ReadingList.getItems() is async, and native menus don't get updated after
> the popupshowing event handler has returned. Does this really need to be
> asynchronous?

Er, oh. Hmm. That API does need to be async, but we could have a separate synchronous API that corresponds to a dedicated cache for the top X items. Most usage is likely to relate to the top X items (10-15), so it would make sense to guarantee those were cached anyway.
Flags: needinfo?(adw)
Comment on attachment 8564264 [details] [diff] [review]
WIP1

Review of attachment 8564264 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-menubar.inc
@@ +451,5 @@
> +            container="true">
> +        <menupopup id="readingListPopup"
> +                   onpopupshowing="onReadingListPopupShowing(this);">
> +          <menuitem id="viewReadingListSidebar" class="subviewbutton"
> +                    oncommand="toggleSidebar('readingListSidebar');"

Bug 1123517 makes this SidebarUI.toggle()

::: browser/base/content/browser-readinglist.js
@@ +5,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "ReadingList",
> +  "resource:///modules/ReadingList.jsm");
> +
> +function onReadingListPopupShowing(aTarget) {

This can go on bug 1123517's ReadingListUI object in this file.
Attachment #8564264 - Flags: feedback?(bmcbride) → feedback+
It just occurred to me that this menu may feel broken if there are no items in the ReadingList, and there's nothing in it's place.

For normal bookmarks folders that are empty, we show a disabled menuitem with the label "(Empty)".

Michael: Do we want to be consistent with empty bookmarks folder, or show something a bit more descriptive? (We need to sort this out now, so we can get the strings landed).
Flags: needinfo?(mmaslaney)
Blair, let's be consistent with the empty bookmarks folder.
Flags: needinfo?(mmaslaney)

Comment 11

3 years ago
Thanks for the heads-up.  Yeah, a small sync cache sounds fine, although it can never be entirely sync because populating it initially from storage must be async.  But after that it'd be sync.
Flags: needinfo?(adw)
Created attachment 8566251 [details] [diff] [review]
Patch v2

I ifdef'ed out the menu on Mac as we can't fill it asynchronously, and can't display an SVG icon on it. I think we should fix these 2 issues in follow-ups.

Other changes:
- updated to address review comments and adapt to the changes done in bug 1123517.
- displaying "(empty)" in the menupopup works
- all these new UI elements are hidden when browser.readinglist.enabled is false
- favicons are shown
- clicking items loads them, like for other bookmarks. The tooltip (showing title and url) and context menu (letting the user open the link in a new tab or window) also works.
- I handled the case of the Bookmarks toolbar button being moved to the menu-panel. In that case the readinglist menu item is just a checkbox to show/hide the sidebar. That's consistent with what the rest of the Bookmark panel does in that case.
Attachment #8564264 - Attachment is obsolete: true
Attachment #8566251 - Flags: review?(bmcbride)
Comment on attachment 8566251 [details] [diff] [review]
Patch v2

Review of attachment 8566251 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, this doesn't look great on Windows: http://d.pr/i/7tzn
Looks like you need placespopup="true" on the <menupopup> for the general popup styling. Though that attribute is also used by browser-places.js - so better check that out.
And the icon isn't showing for some reason.

Can you file the various followups for this as dependent bugs? Including test coverage.

::: browser/base/content/browser-readinglist.js
@@ +6,5 @@
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "ReadingList",
> +  "resource:///modules/readinglist/ReadingList.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "Favicons",

This would be better added to browser.js, I think - since it's so generic (ie, not related to ReadingList, just used by it).

@@ +65,5 @@
>        SidebarUI.hide();
>      }
>    },
> +
> +  onReadingListPopupShowing(aTarget) {

Nit: We've been moving away from prefixing arguments with "a".

@@ +74,5 @@
> +
> +    while (!aTarget.firstChild.id)
> +      aTarget.firstChild.remove();
> +
> +    let insertPoint = aTarget.firstChild;

This and the firstChild.id check above doesn't feel very robust. Would rather see it explicitly use BMB_viewReadingListSidebar in both cases.

::: browser/base/content/browser.xul
@@ +951,5 @@
> +                         onpopupshowing="ReadingListUI.onReadingListPopupShowing(this);">
> +                <menuitem label="article 1"
> +                          class="menuitem-iconic bookmark-item menuitem-with-favicon subviewbutton"/>
> +                <menuitem label="article 2"
> +                          class="menuitem-iconic bookmark-item menuitem-with-favicon subviewbutton"/>

Presumably these should be removed before landing :)

FWIW, I use the following mock data, set in the browser.readinglist.mockData pref: https://pastebin.mozilla.org/8822492

::: browser/themes/shared/readinglist/readinglist-icon.svg
@@ +4,5 @@
> +<rect x="4.8" y="11.2" fill="#808080" width="11.2" height="3.2"/>
> +<rect x="4.8" y="1.6" fill="#808080" width="11.2" height="3.2"/>
> +<circle fill="#808080" cx="1.6" cy="3.2" r="1.6"/>
> +<circle fill="#808080" cx="1.6" cy="8" r="1.6"/>
> +<circle fill="#808080" cx="1.6" cy="12.8" r="1.6"/>

Nit: Indent properly? Should be readable like any other code.

::: browser/themes/windows/jar.mn
@@ +109,5 @@
>          skin/classic/browser/session-restore.svg                     (../shared/incontent-icons/session-restore.svg)
>          skin/classic/browser/tab-crashed.svg                         (../shared/incontent-icons/tab-crashed.svg)
>          skin/classic/browser/welcome-back.svg                        (../shared/incontent-icons/welcome-back.svg)
>          skin/classic/browser/reader-mode-16.png                      (../shared/reader/reader-mode-16.png)
> +        skin/classic/browser/readinglist-icon.svg                    (../shared/readinglist/readinglist-icon.svg)

This should be in /readinglist/, like sidebar.css. There will be a few assets going in there.
Attachment #8566251 - Flags: review?(bmcbride) → review+
Created attachment 8567088 [details] [diff] [review]
Patch v3

(In reply to Blair McBride [:Unfocused] from comment #13)

> Can you file the various followups for this as dependent bugs? Including
> test coverage.

Will do once this lands.

> > +    while (!aTarget.firstChild.id)
> > +      aTarget.firstChild.remove();
> > +
> > +    let insertPoint = aTarget.firstChild;
> 
> This and the firstChild.id check above doesn't feel very robust.

All the nodes from the markup of the xul file have an id attribute; all the generated nodes don't.

> Would
> rather see it explicitly use BMB_viewReadingListSidebar in both cases.

This code path is used by both the menubar bookmarks menu and the bookmarks panel, so there are 2 different ids.


> ::: browser/base/content/browser.xul
> @@ +951,5 @@
> > +                         onpopupshowing="ReadingListUI.onReadingListPopupShowing(this);">
> > +                <menuitem label="article 1"
> > +                          class="menuitem-iconic bookmark-item menuitem-with-favicon subviewbutton"/>
> > +                <menuitem label="article 2"
> > +                          class="menuitem-iconic bookmark-item menuitem-with-favicon subviewbutton"/>
> 
> Presumably these should be removed before landing :)

That was actually dead code; it was removed automatically by the popupshowing handler.

> FWIW, I use the following mock data, set in the browser.readinglist.mockData
> pref: https://pastebin.mozilla.org/8822492

I was already using this that I somehow found in another bug.

Other comments addressed, and Windows theming issues fixed.
Attachment #8566251 - Attachment is obsolete: true
Attachment #8567088 - Flags: review+
Mass change of ReadingList bugs, moving to their own component. Filter bugspam on the following quote:

“Reading is to the mind what exercise is to the body.”
― Joseph Addison
Component: General → Reading List
Duplicate of this bug: 1126113
https://hg.mozilla.org/integration/fx-team/rev/46aebcd9481e
Flags: qe-verify? → qe-verify+
https://hg.mozilla.org/mozilla-central/rev/46aebcd9481e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Target Milestone: Firefox 38 → Firefox 39
QA Contact: andrei.vaida
Comment on attachment 8567088 [details] [diff] [review]
Patch v3

Approval Request Comment
[Feature/regressing bug #]: part of the readinglist feature we are building for 38.
[User impact if declined]: key part of the feature
[Describe test coverage new/current, TreeHerder]: No automated tests at this point. QA will verify.
[Risks and why]: reasonable as it's very early aurora.
[String/UUID change made/needed]: none. The new string was pre-landed.
Attachment #8567088 - Flags: approval-mozilla-aurora?
status-firefox38: --- → affected
tracking-firefox38: --- → +
This is verified fixed on Nightly 39.0a1 (2015-02-25), keeping in mind that the issues from Comment 12 will be treated in separate bugs. Testing was done using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
status-firefox39: fixed → verified
Comment on attachment 8567088 [details] [diff] [review]
Patch v3

Approving for Aurora uplift and currently any bugs that only impact reading list feature (and don't have strings) can land with a=readinglist for the first two weeks of Aurora.
Attachment #8567088 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3095f0eb1b3c
status-firefox38: affected → fixed

Updated

2 years ago
Depends on: 1137231
Verified fixed on Aurora 38.0a2 (2015-03-02) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
status-firefox38: fixed → verified
Depends on: 1141353
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.