Closed Bug 598006 Opened 14 years ago Closed 13 years ago

"Character Encoding" under Menu Bar does not work correctly.

Categories

(Firefox :: Menus, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b10
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: bugmozz, Assigned: Gavin)

References

Details

(Keywords: regression, Whiteboard: [hardblocker][fx4-fixed-bugday])

Attachments

(2 files)

Attached image screenshot
[STR]
open amazon.com (ISO-8859-1)
see "Character Encoding"
[STR]
It happens after Firefox Button's "Character Encoding " is performed once.

Because *same IDs* are used in those RDF generated menuitems, the problem happens.
Blocks: 583386
Blocks: 575182
No longer blocks: 583386
blocking2.0: --- → ?
:(
blocking2.0: ? → betaN+
Hardware: x86 → All
So ignoring the "duplicate ID" issue, we could perhaps fix this by having UpdateCurrentCharset() use event.target.querySelector() rather than document.getElementById (I'm assuming that a scoped querySelector isn't affected by multiple elements with the same ID).

As for fixing the duplicate ID issue... that almost certainly would involve changes to nsCharsetMenu.cpp, since it actually builds the menu. We could potentially have CreateMenu pass in an ID prefix to the observer somehow (optional data appended to the "node" param?), and then have it use that when building the menu. There seems to be some existing "ID prefix" handling that we could perhaps re-use.
(In reply to comment #3)
> As for fixing the duplicate ID issue... that almost certainly would involve
> changes to nsCharsetMenu.cpp

I spent some time investigating this, and ended up spiraling down a rabbit hole of disgusting hacks and RDF evilness (https://hg.mozilla.org/users/gsharp_mozilla.com/patches/annotate/466b7ddcd07b/charsetMenu if you're interested, but you shouldn't be - it's gross and has a bunch of problems).

I have a patch that implements the fix from the first part of comment 3, and it seems to work OK.
Assignee: nobody → gavin.sharp
Attached patch patchSplinter Review
This code is horribly crufty and broken. I'm going to file followups on fixing it properly, but that is clearly not a Firefox 4 thing.

As mentioned, this doesn't attempt to solve the duplicate-ID problem, but rather just scopes the "update" functions to only operate on the menu being used.

I had to make another change to get things working correctly: I adjusted the calls to CreateMenu/UpdateMenus in the onpopupshowing/onpopupshown handlers. This was _really_ messed up, and seems to have been done this way to account for ancient menu updating bugs that no longer exist. CreateMenu calls into the RDF code to build the menu, while UpdateMenus() updates selected items as needed. UpdateMenus was being called before the second CreateMenu(), but ended up working anyways because it repeated its work in a 0ms timeout. I removed that and just reworked things so that we first call both CreateMenu()s and then call UpdateMenus() (from onpopupshown). It's still very inefficient to call these for all popupshowing/popupshown events (including those fired for sub-menus), but I didn't want to mess with this too much.

One other change is the fixing the code to properly handle an empty value for the intl.charset.detector pref - should treat that as "off". This was an existing bug (on current trunk, no entry will be selected in the charset detector menu on first run), but it's made worse by the fact that there are now multiple menus, so even if you explicitly select one in one of the menus, the other menu won't get updated as it should.

This code would benefit from some basic test coverage, but I wanted to get this up for review while I worked on that.
Attachment #502961 - Flags: review?(dolske)
Comment on attachment 502961 [details] [diff] [review]
patch

Looks like some of this code also exists over in toolkit, should we change that too?
Attachment #502961 - Flags: review?(dolske) → review+
I'd rather avoid touching it. I think it's dead code, but even if it isn't it's quite unlikely to be used in a situation where multiple menus exist - that's pretty Firefox-specific.
Whiteboard: [hardblocker]
https://hg.mozilla.org/mozilla-central/rev/f7266ba416f6

I have a partial test for this, but the menubar/menubutton platform inconsistencies makes it tricky to get right. Will keep it in my queue and get it landed when I can. Litmus coverage for this feature in general would be good, too.
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
I tried to follow the steps from comment 1 but I'm not able to reproduce this problem with a b7 build on Windows 7. Can someone please give exact steps which have to be done to raise this issue? Thanks.
Henrik: the problem is that the two menus get out of sync in general - options selected in one aren't reflected in the other. And switching between pages with different encodings (e.g. about:config and http://www.amazon.com/) results in the menu sometimes not showing the correct encoding.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
+  var menuitem = charsetMenuGetElement(target, "chardet." + prefvalue);

shouldn't be "charset"?
Verified ok with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11) Gecko/20100101 Firefox/4.0b11 ID:20110203141415
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker] → [hardblocker][fx4-fixed-bugday]
(In reply to comment #11)
> +  var menuitem = charsetMenuGetElement(target, "chardet." + prefvalue);
> 
> shouldn't be "charset"?

No, it's "chardet" for the auto-detect menu, and "charset" for the others. That non-obvious distinction caused some frustration while writing the patch!
You need to log in before you can comment on or make changes to this bug.