985172 - When the bookmark button is in the overflow menu the label doesn't toggle between "Bookmark this page" and "Edit this bookmark".

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[u482975]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140316030202

Steps to reproduce:

I created a small enough window so that the bookmark button is pushed into the overflow menu. I then used the bookmarking button in the overflow menu to create a bookmark.


Actual results:

The label of the bookmark button with the text "Bookmark this page" didn't change to "Edit this bookmark".


Expected results:

The button should change its text label to represent the functionality it provides.

Comment 1

[:Gijs (he/him)]

Can you reproduce this issue in safe mode (ie with add-ons disabled) and if not, can you isolate which add-on is causing this issue? Thank you!

Comment 2

[u482975]

Yes, the issue is present in safe mode aswell.

Comment 3

[:Gijs (he/him)]

Attachment: toggle bookmark label when in overflow panel,

Not really sure how we didn't notice this before beta, but here we are... :|

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8393218 [details] [diff] [review]
toggle bookmark label when in overflow panel,

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

Looks like we may have done a better use of bookmarkThisPageBroadcaster to update both the menuitem and the overflowed label... though not at this point (it may be filed for future thoughts). This patch is fine for beta.

I think we didn't notice this until beta cause the overflow case is rare, due to wide screens (do we have telemetry about percentage of users with an overflowed toolbar?)

Comment 5

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #4)
> Comment on attachment 8393218 [details] [diff] [review]
> toggle bookmark label when in overflow panel,
> 
> Review of attachment 8393218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like we may have done a better use of bookmarkThisPageBroadcaster to
> update both the menuitem and the overflowed label... though not at this
> point (it may be filed for future thoughts). This patch is fine for beta.

The reason I didn't do that is because the label needs to be 'Bookmarks' for when it's in the panel... I guess we could removeAttribute/setAttribute that and at the same time meddle with the observes attribute? Maybe? Well, we need to rethink this whole widget anyway. :-(

> 
> I think we didn't notice this until beta cause the overflow case is rare,
> due to wide screens (do we have telemetry about percentage of users with an
> overflowed toolbar?)

Sadly, no.

Comment 6

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/119cee2680e0

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/119cee2680e0

Comment 8

[:Gijs (he/him)]

Comment on attachment 8393218 [details] [diff] [review]
toggle bookmark label when in overflow panel,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: Bookmark button label can be wrong in overflow panel
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low, very specific fix
String or IDL/UUID changes made by this patch: none

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e1b07f5a12a1
https://hg.mozilla.org/releases/mozilla-beta/rev/846e9f1e982b

Comment 10

[Francesca Ciceri [:madamezou]]

Hi,

I was able to reproduce it on Nightly 31.0a1 2014-03-18 on Windows 7 x86_64 and I can confirm that the bug is fixed on latest Nightly (32.0a1 2014-05-29), latest Aurora (31.0a2 2014-05-29) and latest Beta (30.0 2014-05-27) on the same platform.

Cheers,
Francesca