1404999 - Cannot click "properties" of a bookmark in bookmark-toolbar while bookmark website is loading.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dan]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170926190823

Steps to reproduce:

Click bookmark left click, then instantly right click, "properties" will be grey so not able to click it.


Actual results:

properties is grey


Expected results:

properties window should be able to be opened while the website of the bookmark is loading.

Comment 1

[YF (Yang)]

WFM in Fx56 x86 on Win10.

Comment 2

[Dan]

I have 64 bit Win 10 creators update but not fall creators update. you?

Comment 3

[Dan]

And you have Firefox on SSD? I have it installed on HDD which needs a few ms to read/write... But haven't had this issue in FF 54 and iirc neither in 55...

Comment 4

[YF (Yang)]

Win10 1703 on HDD.


Try https://mozilla.github.io/mozregression/.

Comment 5

[Mark Banner (:standard8)]

I think this could be related to bug 1395619 - which is an intermittent failure we've been seeing.

I can't reproduce directly on my Mac, but I do seem to be able to get this to reproduce 100%:

1) Have some pinned tabs & normal tabs open. Have some bookmarks in the bookmarks toolbar.
2) Select one of the pinned tabs (so that selecting a bookmark on the toolbar will open in new tab).
3) Click a bookmark, and immediately right-click

Actual results

=> The context menu is entirely disabled

Expected results

=> The context menu is enabled

Comment 6

[Valentina Claudia Ona]

Hi, Daniel 
Can you please tell me if the issue is reproducible, using Latest Nightly 58.0a1?

Comment 7

[Mark Banner (:standard8)]

For my steps, mozregression says:

36:05.23 INFO: Last good revision: 9be2eb8c9061cdfc16d7d53e8f384afa5e1dcb8e
36:05.23 INFO: First bad revision: 56b70ce1a2ab369dd11b86d13d2966f8173ca356
36:05.23 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9be2eb8c9061cdfc16d7d53e8f384afa5e1dcb8e&tochange=56b70ce1a2ab369dd11b86d13d2966f8173ca356

I suspect bug 1354564, but I'm trying to confirm that.

I would suspect that this is directly related to Daniel's issue - it just depends on timing.

Comment 8

[Mark Banner (:standard8)]

I confirmed by local backout that the steps in comment 5 are caused by bug 1354564. Given the nature of that bug, I think it could cause the main issue here, and cause the race that we're seeing in bug 1395619.

Comment 9

[Neil Deakin]

Attachment: Force command updating to be enabled when a menu is opened.

This is the simplest fix.

Comment 10

[Dan]

Can this be fixed for FF Beta 57.0b6 this friday? :)

Comment 11

[Mark Banner (:standard8)]

I've just tested it locally and the patch seems to work. I also ran it through the unit tests about 10 times, and it didn't fail (I'd normally have expected bug 1395619 to be showing up within a few runs as well).

Comment 12

[Dan]

When is the earliest date (this friday?) for this patch/fix to be included in FF Beta?

Comment 13

[Neil Deakin]

Comment on attachment 8914734 [details] [diff] [review]
Force command updating to be enabled when a menu is opened.

OK, don't see any issues with this.

I don't see any reason to rush this into 57; the issue has been present for several versions.

Comment 14

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Daniel, maybe next week :)
Too late for a fix in Firefox 56 though.

Comment 15

[Dan]

oh ok! :) Next week, great!

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8914734 [details] [diff] [review]
Force command updating to be enabled when a menu is opened.

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

Thanks, Enn!

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js
@@ +133,5 @@
> +// enabled state of the menuitems is updated properly.
> +add_task(async function quickContextMenu() {
> +  await SpecialPowers.pushPrefEnv({set: [
> +    [PREF_LOAD_BOOKMARKS_IN_TABS, true]
> +  ]})

Nit: missing semi-colon, which I think is going to burn you with the latest ESLint stuff.

@@ +150,5 @@
> +    type: "contextmenu"
> +  });
> +  await promise;
> +
> +  Assert.notEqual(document.getElementById("placesContext_open").disabled, true,

Alternatively:

Assert.ok(!document.getElementById("placesContext_open").disabled,
          "Commands in context menu are enabled");

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04daa6d238c7
force command updating to be enabled when a menu is opened, r=mconley

Comment 18

[Dan]

awesooome! :) Thanks so much! Can't wait for it to go into FF Beta soon!

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/04daa6d238c7

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Will ride the train with 58

Comment 21

[Dan]

Thanks so much! Can we get this into FF 57 Beta 10 this friday pleeeease Sebastian? :)

Comment 22

[Dan]

cannot wait another 31 days for this to be fixed... -.-

Comment 23

[Mark Banner (:standard8)]

Daniel, I realising this is affecting you quite a bit, but unfortunately we've already deemed this not critical enough to fix for 57. It will be fixed in the next versions that follow soon.

Comment 24

[Dan]

Ah alright! :) Can I expect it in the very first FF 58 beta?

Comment 25

[Mark Banner (:standard8)]

Yes, the status is marked as fixed for FF 58.

Comment 26

[Dan]

When can we expect this in FF Beta? :/ Can't wait! :P