Richard Marti (:Paenglab) Assignee
	Description • 14 years ago

User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; rv:2.0b3pre) Gecko/20100723 Minefield/4.0b3pre
Build Identifier: 

See also the Firefox Bug 456535. This would also help Bug 569400 (Aero Glass on Thunderbird)

Reproducible: Always

	Richard Marti (:Paenglab) Assignee
	Comment 1 • 14 years ago

This patch is a copy paste from the Firefox patch. It only works in Main window. In Addressbook and Composer the menu entries appears but the menu does not hide.

I'm not a coder, so if someone with experience can take this.

	Richard Marti (:Paenglab) Assignee
	Comment 2 • 14 years ago

I tried the patch again and deleted the localstore.rdf. Now it works also in Addressbook and Composer.

But if you need tests, then a developer should take this Bug to create the needed tests.

	Wayne Mery (:wsmwk)
	Comment 4 • 14 years ago

awesome idea

	Ludovic Hirlimann [:Usul]
	Comment 5 • 14 years ago

If you want your patch to get thru you'll need to ask for reviews see https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements

	Richard Marti (:Paenglab) Assignee
	Comment 6 • 14 years ago

Comment on attachment 460121 [details] [diff] [review]
Patch with corrected line endings CR LF -> LF

I hope I took the right one.

If this patch need automated tests, I hope someone takes this Bug and writes them. I'm not a programmer and I don't know how to write them.

I tried to assign this Bug to me. But I have no rights to do this or I don't know how to do.

	Jim Porter (:squib)
	Comment 7 • 14 years ago

I assigned it to you.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 8 • 14 years ago

Richard, thanks so much for the patch!

I've adjusted your Bugzilla privs so that you will be able to edit bugs and assign them to yourself in the future.

This patch will need tests before it can land.  Jim, I don't suppose you'd have any interest in putting together an automated test for this bug?

	Dan Mosedale (:dmosedale, :dmose)
	Comment 9 • 14 years ago

Comment on attachment 460121 [details] [diff] [review]
Patch with corrected line endings CR LF -> LF

Removing the review request until we have a patch with ui-review and tests.  Once we do get to that patch, I suspect bwinton is probably the most suitable reviewer here...

	Jim Porter (:squib)
	Comment 10 • 14 years ago

(In reply to comment #8)
> This patch will need tests before it can land.  Jim, I don't suppose you'd have
> any interest in putting together an automated test for this bug?

I haven't really looked at the patch. If it's platform-independent, I can probably do it, but I don't have access to a Windows Vista/7 machine (and my XP machine isn't set up to compile Mozilla stuff).

	Richard Marti (:Paenglab) Assignee
	Comment 11 • 14 years ago

Jim, maybe if you remove the #ifdef XP_WIN in the .xul files it should also work on other platforms.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 12 • 14 years ago

Once this gets ui-review, please remind me and I'll try and find someone to help out with the unit test.

	Andreas Nilsson (:andreasn)
	Comment 13 • 14 years ago

Comment on attachment 460121 [details] [diff] [review]
Patch with corrected line endings CR LF -> LF

"Stealing" Bryan's UI-review (after asking him on IRC)

	Wayne Mery (:wsmwk)
	Comment 14 • 14 years ago

also already available via https://addons.mozilla.org/en-US/thunderbird/addon/4762/ if the write up is correct

	Andreas Nilsson (:andreasn)
	Comment 15 • 14 years ago

For some odd reason I got the extension to work, but not the patch. Investigating what could be going on.

	Richard Marti (:Paenglab) Assignee
	Comment 16 • 14 years ago

Did you test it with AeroGlass theme? Then remove the -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");

With this in the theme the autohide does not work. If you don't use this theme, then try it with deleting or renaming of the localstore.rdf.

	Andreas Nilsson (:andreasn)
	Comment 17 • 14 years ago

Comment on attachment 460121 [details] [diff] [review]
Patch with corrected line endings CR LF -> LF

That did the trick!
It's good to see that it's needs an initial trigger from a right click menu in order to work, meaning that it won't be triggered by people who don't know of the function.
ui-r+!

	Ludovic Hirlimann [:Usul]
	Comment 19 • 14 years ago

Why was review request not requested ?

	Richard Marti (:Paenglab) Assignee
	Comment 20 • 14 years ago

(In reply to comment #19)
> Why was review request not requested ?

Because I don't know how to write the required automated test and nobody offered to do it.

	Blake Winton (:bwinton) (:☕️)
	Comment 21 • 14 years ago

Richard,

I could probably help you write the tests.  Can you ping me (bwinton) on IRC sometime today?

(Also, I'm assigning it to you, cause it looks like you've done most of the work on it already.  ;)

Later,
Blake.

	Jim Porter (:squib)
	Comment 22 • 13 years ago

Just for tracking purposes, here are some tests I wrote for this bug. However, not having a Windows dev environment, I can't actually test the tests, so hopefully someone can step in and polish this off. :)

	Steffen Wilberg
	Comment 23 • 13 years ago

Why do you limit that to Windows? Firefox allows to hide the menubar on Linux as well (and displays the Firefox button).

	Richard Marti (:Paenglab) Assignee
	Comment 24 • 13 years ago

(In reply to comment #23)
> Why do you limit that to Windows? Firefox allows to hide the menubar on
> Linux as well (and displays the Firefox button).

On Vista/Win7 it is usual habit to hide the menubar, on Linux not. But FX has the AppMenu button which is showing when the menubar is hidden. TB don't have this ATM and this would it make hard to discover the new behaviour on Linux.

	Richard Marti (:Paenglab) Assignee
	Comment 25 • 13 years ago

Comment on attachment 543870 [details] [diff] [review]
WIP patch with tests

Blake can you run the tests and check if they are working?
If they are working, this is also a r? ;)

	Jim Porter (:squib)
	Comment 26 • 13 years ago

(In reply to comment #23)
> Why do you limit that to Windows? Firefox allows to hide the menubar on
> Linux as well (and displays the Firefox button).

That's dependent on having a Thunderbird button, which is beyond the scope of this bug.

	David :Bienvenu
	Comment 27 • 13 years ago

(In reply to comment #26)

> That's dependent on having a Thunderbird button, which is beyond the scope
> of this bug.

Do we have a bug for that?

	Rain [:rain1] (inactive)
	Comment 28 • 13 years ago

I figured out why the tests aren't working: to summarize, the click calls don't flip the checkmark, so our code gets confused. See bug 670829 and bug 670830.

	Rain [:rain1] (inactive)
	Comment 29 • 13 years ago

Wow, we still have lots of issues with our tests :/

I've made this depend on MozMill 1.5 because it seemed to be the most convenient thing to do.

	Blake Winton (:bwinton) (:☕️)
	Comment 30 • 13 years ago

Comment on attachment 545613 [details] [diff] [review]
patch with working tests

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

Other than those small nits, and the fact that I can't run the tests because we're in the middle of upgrading MozMill, I like it!  ;)

So, r=me with those fixed, and the test infrastructure upgrade finished.

Thanks,
Blake.

::: mail/base/content/mailCore.js
@@ -269,2 +269,4 @@
> > -  toolbar.collapsed = aEvent.originalTarget.getAttribute("checked") != "true";
> > +  var hidingAttribute = toolbar.getAttribute("type") == "menubar" ?
> > -  document.persist(toolbar.id, "collapsed");
> > +                        "autohide" : "collapsed";
> > +
> > +  dump("final checked = " + aEvent.originalTarget.getAttribute("checked"));

We should probably remove the dump statement here.

::: mail/test/mozmill/message-window/test-autohide-menubar.js
@@ +18,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2011
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Jim Porter <squibblyflabbetydoo@gmail.com>

Should this be you or Squib?

@@ +67,5 @@
> + *
> + * @param controller the mozmill controller for the window
> + * @param elem the element to click on (usually the menubar)
> + * @param hide true to hide, false otherwise
> + * @return true if the menubar's autohide attribute could be set

This function doesn't seem to ever return true!

@@ +90,5 @@
> +function help_test_autohide(controller, menubar) {
> +  function hiddenChecker(aHidden) {
> +    // The XUL hidden attribute isn't what is set, so it's useless here -- use
> +    // information from the box model instead.
> +    return function () ((menubar.getBoundingClientRect().height != 0) ^ aHidden);

I think I'ld prefer "==" to "^".  (Or is that "!="?)

	Rain [:rain1] (inactive)
	Comment 31 • 13 years ago

https://hg.mozilla.org/comm-central/rev/8bbd7b60e5a8

	Rain [:rain1] (inactive)
	Comment 32 • 13 years ago

No idea what happened...

	Rain [:rain1] (inactive)
	Comment 33 • 13 years ago

Pushed a followup adding the tests I forgot to add in the previous push: https://hg.mozilla.org/comm-central/rev/e5a042f2e020

No idea how I missed them though :(

	Charles
	Comment 34 • 13 years ago

(In reply to David :Bienvenu from comment #27)
> (In reply to comment #26)
> 
> > That's dependent on having a Thunderbird button, which is beyond the scope
> > of this bug.
> 
> Do we have a bug for that?

https://bugzilla.mozilla.org/show_bug.cgi?id=650170