Closed
Bug 598006
Opened 14 years ago
Closed 13 years ago
"Character Encoding" under Menu Bar does not work correctly.
Categories
(Firefox :: Menus, defect)
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)
196.39 KB,
image/jpeg
|
Details | |
3.60 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
[STR] open amazon.com (ISO-8859-1) see "Character Encoding"
Comment 1•14 years ago
|
||
[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.
Updated•14 years ago
|
Updated•14 years ago
|
blocking2.0: --- → ?
Assignee | ||
Comment 3•14 years ago
|
||
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.
Assignee | ||
Comment 4•14 years ago
|
||
(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 | ||
Updated•14 years ago
|
Assignee: nobody → gavin.sharp
Assignee | ||
Comment 5•13 years ago
|
||
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 6•13 years ago
|
||
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+
Assignee | ||
Comment 7•13 years ago
|
||
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.
Updated•13 years ago
|
Whiteboard: [hardblocker]
Assignee | ||
Comment 8•13 years ago
|
||
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
Comment 9•13 years ago
|
||
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.
Assignee | ||
Comment 10•13 years ago
|
||
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 → ---
Assignee | ||
Updated•13 years ago
|
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 11•13 years ago
|
||
+ var menuitem = charsetMenuGetElement(target, "chardet." + prefvalue); shouldn't be "charset"?
Comment 12•13 years ago
|
||
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]
Assignee | ||
Comment 13•13 years ago
|
||
(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.
Description
•