Closed Bug 637311 Opened 13 years ago Closed 8 years ago

Checkmark not displayed for current encoding in View > Character Encoding when menu has been opened the first time

Categories

(Firefox :: Menus, defect, P4)

All
macOS
defect

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: fvsch, Assigned: stefanh)

Details

(Keywords: reproducible, Whiteboard: [4rc][tpi:+])

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12) Gecko/20100101 Firefox/4.0b12
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

When using the View > Character Encoding menu for the first time in a Firefox session, or for the first time after opening a new window, Firefox does not display a checkmark before the currently used encoding.
The View > Character Encoding > Auto-Detect menu has the same issue.

I'll attach a screenshot.

Problem seen on OS X with FF4b12.
Seen by a friend on Archlinux i686 with FF4b11.

Reproducible: Always

Steps to Reproduce:
1. Start Firefox and open any page.
2. Go to the View > Character Encoding menu
Actual Results:  
The current encoding (whether declared or auto-detected) should be shown with a check mark.

Expected Results:  
No encoding is highlighted.
Attached image Combined screenshots
Able to reproduce using  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b13pre) Gecko/20110228 Firefox/4.0b13pre.
Status: UNCONFIRMED → NEW
Ever confirmed: true
That's a regression. Would be great to have a regression date available. Teodosia, would you have time for such a check?
Severity: minor → normal
Hardware: x86 → All
Whiteboard: [4rc]
Version: unspecified → Trunk
As a quick check shows it only happens the first time the menu has been used. Each successive attempt will succeed.
Severity: normal → minor
Summary: Checkmark not displayed for current encoding in View > Character Encoding → Checkmark not displayed for current encoding in View > Character Encoding when menu has been opened the first time
Works for me on trunk on Linux. Is this bug still present on Mac? (Note that as of Firefox 28, some unusual encodings are no longer in the menu, so you need to test with the page that uses one of the encodings that are still in the menu.)
This is still present on OS X with a Nightly build from 20130110.
OK. This is then probably a bug in how XUL menus are reflected as Mac native menus rather than a bug in the Character Encoding menu implementation.
Component: Menus → Widget: Cocoa
Product: Firefox → Core
I can reproduce this if FF 26 on OS X 10.8.5.  How bizarre!

But as this is just a minor annoyance, it may take a while for somebody to get to it.
This is so old it's no longer really a "regression".  That doesn't mean we shouldn't fix it, though.
I'm going to look at this since I have a theory about the problem.
Assignee: nobody → stefanh
Priority: -- → P4
Whiteboard: [4rc] → [4rc][tpi:+]
So the problem here is that we set the checkmark after the menu have been shown: https://dxr.mozilla.org/comm-central/rev/5639a9f476d08f300c079117e61697f5026b6367/mozilla/browser/base/content/browser-charsetmenu.inc#9-10.

That doesn't work on Mac since you can't update a Mac menu when it's visible. The obvious solution is to call UpdateCurrentCharset(this) when the popupshowing event fires (after CharsetMenu.build(event.target)).

Gijs isn't back until the 26:th, but I'll attach a patch that he can look at when he's back.
ni? myself in order to remember requesting review.
Flags: needinfo?(stefanh)
Status: NEW → ASSIGNED
Flags: needinfo?(stefanh)
Flags: needinfo?(stefanh)
Comment on attachment 8803650 [details] [diff] [review]
Update charset menu before it's visible

Gijs, mind take a look at this? Info is in comment #11. One alternative is to only change this for mac (ifdef).
Comment on attachment 8803650 [details] [diff] [review]
Update charset menu before it's visible

Gijs, mind take a look at this? Info is in comment #11. One alternative is
to only change this for mac (ifdef).

(this time with a review request)
Flags: needinfo?(stefanh)
Attachment #8803650 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8803650 [details] [diff] [review]
Update charset menu before it's visible

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

Apparently this has been iffy since at least Firefox 4 (bug 598006). Huh.
Attachment #8803650 - Flags: review?(gijskruitbosch+bugs) → review+
Component: Widget: Cocoa → Menus
Product: Core → Firefox
Pushed by stefanh@inbox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a468e41c48de
Update charset menu before it's visible so the checkmark for current encoding is displayed on Mac the first time the menu is opened. r=Gijs.
https://hg.mozilla.org/mozilla-central/rev/a468e41c48de
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Verified fixed on Fx 52.0b7 Ubuntu 16.04
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: