Closed Bug 1248266 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox47 --- verified

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: access)

Attachments

(2 files)

This is because of the vbox used to contain the items...
Attached patch patchSplinter Review
this applies after bug 1248258's patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8719298 - Flags: review?(mak77)
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?
Attachment #8719298 - Flags: review?(mak77) → review+
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.
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.
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.
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.
(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
https://hg.mozilla.org/mozilla-central/rev/6d7cd27562d4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Attached image 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?
Flags: needinfo?(dao)
(In reply to Mihai Boldan, QA [:mboldan] from comment #10)
> Should I file new bugs for this two issues?

Yes please.
Flags: needinfo?(dao)
Depends on: 1250806
Depends on: 1250812
Since the found issues were logged separately as Bug 1250806 and Bug 1250812, I am marking this issue Verified-Fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.