[de-xbl] remove the toolbar-menubar-autohide binding
Categories
(Thunderbird :: Toolbars and Tabs, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: khushil324)
References
Details
Attachments
(2 files, 5 obsolete files)
|
14.02 KB,
patch
|
Details | Diff | Splinter Review | |
|
14.70 KB,
patch
|
khushil324
:
review+
jorgk-bmo
:
feedback+
|
Details | Diff | Splinter Review |
toolbar-menubar-autohide is not used a lot, I think only for the main toolbar. Hooked up here: https://searchfox.org/comm-central/rev/024094c2ebc05723d2b0da86086539d5b97044cb/mail/base/content/bindings.css#19
It is used in tests at https://searchfox.org/comm-central/rev/024094c2ebc05723d2b0da86086539d5b97044cb/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#2877 and set in https://searchfox.org/comm-central/source/mail/base/modules/MailMigrator.jsm#112 and in migration at https://searchfox.org/comm-central/rev/024094c2ebc05723d2b0da86086539d5b97044cb/mail/base/modules/MailMigrator.jsm#111
See also
- https://searchfox.org/comm-central/search?q=mail.main_menu.collapse_by_default&path=
- https://searchfox.org/comm-central/search?q=setAttribute(%22autohide%22&case=false®exp=false&path=
For Firefox, this got removed in bug 1499236 - so we can pretty much do what they did there: https://hg.mozilla.org/mozilla-central/rev/5c84e7821c57
|
Reporter | |
Updated•5 years ago
|
|
Assignee | |
Comment 1•5 years ago
|
||
|
|
Assignee | |
Updated•5 years ago
|
|
|
Reporter | |
Comment 2•5 years ago
|
||
Comment on attachment 9072505 [details] [diff] [review] Bug-1546336_de-xbl_toolbar-menubar-autohide.patch Review of attachment 9072505 [details] [diff] [review]: ----------------------------------------------------------------- The build complains about there no longer being any preprocessing in the bindings.css file. So you should remove the star from https://searchfox.org/comm-central/source/mail/base/jar.mn#43 (which means preprocess this file) Can we put the file in mail/base/content/messenger-customization.js to match up with firefox? Doesn't seem to work though. If I right click the toolbar and uncheck the menu bar, the menu bar still shows. That's what this binding is about, isn't it?
|
|
Assignee | |
Comment 3•5 years ago
•
|
||
(In reply to Magnus Melin [:mkmelin] from comment #2)
The build complains about there no longer being any preprocessing in the
bindings.css file. So you should remove the star from
https://searchfox.org/comm-central/source/mail/base/jar.mn#43 (which means
preprocess this file)
Okay, Cool.
Can we put the file in mail/base/content/messenger-customization.js to match
up with firefox?
Yeah, sure.
Doesn't seem to work though. If I right click the toolbar and uncheck the
menu bar, the menu bar still shows. That's what this binding is about, isn't
it?
Yes, I checked on Linux Machine and it was working. I will check again.
|
|
Assignee | |
Comment 4•5 years ago
|
||
|
|
Reporter | |
Updated•5 years ago
|
|
|
Reporter | |
Comment 5•5 years ago
|
||
Comment on attachment 9072808 [details] [diff] [review] Bug-1546336_de-xbl_toolbar-menubar-autohide.patch Review of attachment 9072808 [details] [diff] [review]: ----------------------------------------------------------------- The code looks ok but hiding/showing the menu toolbar doesn't work on linux at least.
|
|
Assignee | |
Comment 6•5 years ago
|
||
|
|
Reporter | |
Comment 7•5 years ago
|
||
Comment on attachment 9072988 [details] [diff] [review] Bug-1546336_dexbl_toolbar-menubar-autohide.patch Review of attachment 9072988 [details] [diff] [review]: ----------------------------------------------------------------- Works now, thx! r=mkmelin assuming the tests are happy ::: mail/base/content/messenger-customization.js @@ +26,5 @@ > + this.contextMenu = document.getElementById(contextMenuId); > + this.contextMenu.addEventListener("popupshown", this); > + this.contextMenu.addEventListener("popuphiding", this); > + AutoHideMenubar._node.addEventListener("mousemove", this); > + }, nit: please add an empty line between the functions here
|
|
Assignee | |
Comment 8•5 years ago
|
||
|
|
Assignee | |
Comment 9•5 years ago
|
||
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 10•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e0efa65d3901
[de-xbl] remove the toolbar-menubar-autohide binding. r=mkmelin
|
||
Updated•5 years ago
|
|
|
||
Comment 11•5 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/d49c04d9b70e bug 1546340 (merge conflict) - Follow-up: toolbar.xml doesn't need pre-processing. r=me
|
|
||
Comment 12•5 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/8c4083db9028 Backed out changesets e0efa65d3901 and d49c04d9b70e for test failures on Windows. a=backout
|
|
||
Comment 13•5 years ago
|
||
It was pretty clear that this functionality was running on Windows, yet, you never did a try run on Windows :-(
The failures first showed up after landing this:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=d49c04d9b70e7493bd5c6938b5045ead17659a59
|
|
||
Comment 14•5 years ago
|
||
That's a backout of the backout which gives you a rebased patch to start from.
|
|
Assignee | |
Comment 15•5 years ago
|
||
The problem was due to some merge conflicts. Now I have resolved them.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bca8e69059a420e34ce8dd06c8b319ae15d4d838
|
|
Assignee | |
Updated•5 years ago
|
|
|
||
Comment 16•5 years ago
|
||
This still fails locally on Windows:
mozmake SOLO_TEST=message-window/test-autohide-menubar.js mozmill-one
and you didn't include Windows in your try :-(
|
|
Assignee | |
Comment 17•5 years ago
|
||
|
|
||
Comment 18•5 years ago
|
||
Comment on attachment 9077433 [details] [diff] [review] Bug-1546336_de-xbl_toolbar-menubar-autohide.patch This works now, I'll land it later.
|
|
||
Comment 19•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4f80c9138a49
[de-xbl] remove the toolbar-menubar-autohide binding. r=mkmelin
|
|
||
Updated•5 years ago
|
Description
•