1248266 - 5 most recently added bookmarks aren't keyboard accessible in the menu bar's Bookmarks menu

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dão Gottwald [:dao]]

This is because of the vbox used to contain the items...

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

this applies after bug 1248258's patch

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8719298 [details] [diff] [review]
patch

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

::: browser/base/content/browser-places.js
@@ +1351,5 @@
>      options.maxResults = kMaxResults;
>      let query = PlacesUtils.history.getNewQuery();
>  
> +    while (aHeaderItem.nextSibling &&
> +           aHeaderItem.nextSibling.localName == "menuitem") {

The only downside is that if anyone in future adds a menuitem it would be removed... while it's unlikely, it would be a quite obscure bug.
what about we set an attribute on the items we are adding and remove until the nextSibling has the attribute set?

Comment 3

[Dão Gottwald [:dao]]

Nobody should be adding a menuitem without adding a separator first, because otherwise it would be grouped with our heading. So I think this can land as-is.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

provided we don't add a link to show all bookmarks ordered by most recent or some other fancy UI change :)
The attribute would be safer, but I'm not blocking on it.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

an attribute/class would also allow to easily hide these items through a userStyle if the users don't like those taking up space. it was easier to hide the vbox.

Comment 6

[Dão Gottwald [:dao]]

Sorry, I'm running late (was planning to be on PTO yesterday already), so I'll land this as is and we can discuss separately if we should use a class or add a pref for bug 1248268.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6d7cd27562d4

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #6)
> Sorry, I'm running late (was planning to be on PTO yesterday already), so
> I'll land this as is and we can discuss separately if we should use a class
> or add a pref for bug 1248268.

SGTM

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d7cd27562d4

Comment 10

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: recently bookmarked.png

I managed to reproduce this issue on Firefox 47.0a1 (2016-02-14) and on Windows 10 x86.
During testing, I've encountered two possible issues.
- On Mac OS X, no bookmarks are displayed in Menu -> Bookmarks (see the attached screenshot).
- On Windows, if the keyboard is used to navigate through the Bookmarks, the 'Recently bookmarked' title is selected as it would be a button. On Mac OS X and Ubuntu, the selection jumps over the title. This issue is reproducible if the bookmarks are accessed from Menu->Bookmarks or from 'Show your bookmarks' button from the toolbar.
Should I file new bugs for this two issues?

Comment 11

[Dão Gottwald [:dao]]

(In reply to Mihai Boldan, QA [:mboldan] from comment #10)
> Should I file new bugs for this two issues?

Yes please.

Comment 12

[Mihai Boldan, Desktop QA [:mboldan]]

Since the found issues were logged separately as Bug 1250806 and Bug 1250812, I am marking this issue Verified-Fixed.