Closed Bug 985172 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

29 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- fixed
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified

People

(Reporter: MartijnJoosstens, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 file)

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.
Whiteboard: [Australis:P3]
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!
Flags: needinfo?(MartijnJoosstens)
Yes, the issue is present in safe mode aswell.
Flags: needinfo?(MartijnJoosstens)
Not really sure how we didn't notice this before beta, but here we are... :|
Attachment #8393218 - Flags: review?(mak77)
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Component: Untriaged → Toolbars and Customization
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?)
Attachment #8393218 - Flags: review?(mak77) → review+
(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.
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/119cee2680e0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
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
Attachment #8393218 - Flags: approval-mozilla-beta?
Attachment #8393218 - Flags: approval-mozilla-aurora?
Attachment #8393218 - Flags: approval-mozilla-beta?
Attachment #8393218 - Flags: approval-mozilla-beta+
Attachment #8393218 - Flags: approval-mozilla-aurora?
Attachment #8393218 - Flags: approval-mozilla-aurora+
QA Whiteboard: [good first verify]
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify] [testday-20140530]
You need to log in before you can comment on or make changes to this bug.