1578111 - Opening bookmarks folder within "show your bookmarks" using keyboard and pressing up-arrow results in two highlighted entries the first time

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[B.J. Herbison]

Attachment: Picture of a screen with two highlighted bookmarks

Steps to reproduce with Firefox Nightly 70:

1. Create a new profile. (Optional)
2. Create a new bookmarks folder (e.g., "test") and place more than one bookmark in the folder.
3. Configure and drag the Show your Bookmarks button to the URL bar.
4. Click the Show your Bookmarks button.
5. Open the bookmarks menu with the keyboard (e.g., "t" for "test").
6. Press the up-arrow twice times.

Expected result:
6) The bottom bookmark is highlighted.

Actual result:
6) The top and bottom bookmarks are selected.

Attached is a picture of Firefox with two items highlighted. I have a video demo, but is is 115 MB so I won't upload it unless this can't be reproduced.

Notes:

• Tested on Ubuntu using XFCE.

• If you press the up arrow once the top bookmark is highlighted and "Open All in Tabs" is highlighted and selected, but the highlight isn't obvious.

• After step 6 you can press up- and down-arrow repeatedly and two highlights will be maintained until you select the top bookmark. Afterwards everything proceeds as expected.

• If you have nested bookmark folders you can get two highlighted entries on each layer of the menu.

• I'm not sure if this can be reproduced with the library icon instead of the bookmarks icon.

• This happens if you first open the folder with the keyboard. I don't know it anything similar can happen using a pointing device.

• This happens once for each folder. Opening a folder a second time won't let this be reproduced, but it can be reproduced opening a different folder.

• Restarting Firefox resets each folder and the bug can be reproduced once on each folder.

Comment 1

[Mark Banner (:standard8)]

I can reproduce this on a few of the sub-menus in my "Show your bookmarks" menu. Moving across to menus, as I think this is probably the backend implementation rather than how bookmarks is generating it.

Comment 2

[:Gijs (he/him)]

AIUI the menu implementation lives in XUL land, so moving there, though I guess it's possible this is a widget/GTK invalidation issue.

Comment 3

[B.J. Herbison]

What is the best way to find out why this is happening? It is disconcerting to have the wrong item shown as selected--it makes me think I typed incorrectly.

Comment 4

[Brian Grinstead [:bgrins]]

I'm guessing this is a regression from Bug 1539651 since this is the places-popup UI and it was reported around the same time that Custom Element conversion landed. Alex, could you do a bit of digging to confirm if it's the cause and see what we can do about it?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #4)

I'm guessing this is a regression from Bug 1539651 since this is the places-popup UI and it was reported around the same time that Custom Element conversion landed.

yep, it could be, it'd be interesting to have regression window.

Alex, could you do a bit of digging to confirm if it's the cause and see what we can do about it?

Unfortunately, I failed to reproduce the bug. It works just nice on Windows; arrow up doesn't seem have any effect on OS X, not sure if this is a feature or a bug. I don't have Ubuntu at hands, the platform the bug was originally reported for, to make a check there.

Comment 6

[Alice0775 White]

I can reproduce the issue on Nightly71.0a1 Windows10.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8bd696e50a12db4f6be4ded519ef4f97ed6fed60&tochange=bd7cd9d7b6ade7bf9fa8441a8686f19c36f2a7e5

Comment 7

[Marcia Knous [:marcia]]

Based on Comment 6 and discussion during triage, re-adding the ni for Alex.

Comment 8

[Alice0775 White]

This was fixed in latest Nightly71.0a1

Fixed window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c420af7f34af85867c4afdec0e32a64dbfbda056&tochange=0257933d9e3104536d2352e559c64f6f42808ee0

Fixed by:257933d9e3104536d2352e559c64f6f42808ee0 Alexander Surkov — Bug 1555497 - Remove menupopup binding, r=bgrins

Comment 9

[:Gijs (he/him)]

This regressed on 70 and got fixed on 71 so that leaves 70 affected... :-(

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Alice0775 White from comment #8)

Fixed by:257933d9e3104536d2352e559c64f6f42808ee0 Alexander Surkov — Bug 1555497 - Remove menupopup binding, r=bgrins

interesting, that's why I failed to reproduce it

(In reply to :Gijs (he/him) from comment #9)

This regressed on 70 and got fixed on 71 so that leaves 70 affected... :-(

should we backport the bug then? honestly no ideas why menupoup conversion to CE could affect on places menupoups CEs, I see nothing obvious in the patch.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #10)

(In reply to Alice0775 White from comment #8)

Fixed by:257933d9e3104536d2352e559c64f6f42808ee0 Alexander Surkov — Bug 1555497 - Remove menupopup binding, r=bgrins

interesting, that's why I failed to reproduce it

(In reply to :Gijs (he/him) from comment #9)

This regressed on 70 and got fixed on 71 so that leaves 70 affected... :-(

should we backport the bug then? honestly no ideas why menupoup conversion to CE could affect on places menupoups CEs, I see nothing obvious in the patch.

I'd be hesitant to uplift the whole menupopup Custom Element conversion. We could do it, but I'd just be worried about stability on beta and then needing to uplift fixes for any regressions related to that late in the cycle.

The main thing the menupopup CE conversion would have changed with places menupopup is the lazy SD attachment, right? It's kind of shot in the dark, but maybe it's possible to make just that change directly to beta.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #11)

I'd be hesitant to uplift the whole menupopup Custom Element conversion. We could do it, but I'd just be worried about stability on beta and then needing to uplift fixes for any regressions related to that late in the cycle.

The main thing the menupopup CE conversion would have changed with places menupopup is the lazy SD attachment, right? It's kind of shot in the dark, but maybe it's possible to make just that change directly to beta.

I can do the patch and see if it helps for sure.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:bgrins, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Alexander and Brian, if one of you comes up with a patch that you think isn't too risky for beta, we can still take it for beta 12 or 13. Otherwise it will wait to be fixed in 71.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Liz Henry (:lizzard) from comment #14)

Alexander and Brian, if one of you comes up with a patch that you think isn't too risky for beta, we can still take it for beta 12 or 13. Otherwise it will wait to be fixed in 71.

weird enough, repeating steps from comment #0:

• Click the Show your Bookmarks button.
• Open the bookmarks menu with the keyboard (e.g., "t" for "test").
• Press the up-arrow twice times.

I cant' reproduce on nightly beta either (70.0b13 (64-bit)) on Windows (10Pro). Alice, could you please give another try to check my observations?

Comment 16

[Alice0775 White]

I can reproduce the issue on Firefox70.0b11 Windows10.
However, I cannot reproduce on Firefox70.0b12 and Nightly71.0a1 Windows10.

STR
0. Start Firefox with new profile

1. Enter customize mode, Put "Bookmarks Menu" widget on NavBar. Exit customize mode.
2. Click "Bookmarks Menu" widget and key press "m"
3. Key press [up-arrow] twice times

Fixed window (mozilla-central, autoland):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c420af7f34af85867c4afdec0e32a64dbfbda056&tochange=0257933d9e3104536d2352e559c64f6f42808ee0

Fixed in m-c by: 0257933d9e3104536d2352e559c64f6f42808ee0 Alexander Surkov — Bug 1555497 - Remove menupopup binding, r=bgrins

Fixed window (mozilla-beta):
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=17b4f7b2c9697d78f98a702572864bd96001d1e0&tochange=24de1dfb60861c82f36c75e700d843cf75081adc

Fixed in beta by: Bug 1575138

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Liz Henry (:lizzard) from comment #14)

Alexander and Brian, if one of you comes up with a patch that you think isn't too risky for beta, we can still take it for beta 12 or 13. Otherwise it will wait to be fixed in 71.

Looks like this got fixed in beta 70 already by Bug 1575138 as per Comment 16, so clearing needinfo.