1307773 - Manage Identities window is too tall to fit monitor screen

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #507640 +++

(Quoting Dan Pernokis from bug 507640 comment #64)
> Sorry, we're not done yet...
> 
> I tested this issue on my netbook (which is only 1024x600).  The initial
> screen for Account Settings did appear correctly, with OK/CANCEL buttons
> visible at the bottom -- so I stand by my Comment #62 that part seems to be
> fixed.
> 
> However, after re-reading various comments (in particular Comment #14), I
> confess that I forgot about "Manage Identities".  I tested this just now,
> but sorry to say, the window is still too tall! OK/CANCEL are not visible,
> and neither are the settings on the lower parts of the various tabs.  (It
> makes no difference whether TB is running full-screen or simply filling the
> screen.)
> 
> I have to use my usual work-around (similar to David's Comment #47 for
> Account Settings):
> I try to click on the top margin of the window so it jumps down about 1cm
> and the scroll bar appears.  Then I can resize the window by pulling the top
> margin down substantially (about 1/4 screen height), then grabbing the top
> bar to move the window up to the top.  With the whole window visible, I can
> see OK/CANCEL pinned to the bottom area, while the scroll bar allows me to
> access the full window contents one way or another.
> 
> So please RE-OPEN this bug -- it's only half fixed.  (But I think it's the
> same fix for both: scroll bar needs to appear at the outset if the full
> window is too tall.)

Comment 1

[Magnus Melin [:mkmelin]]

That was a separate bug (can't find it now), but at least for me a scrollbar appears (tried 800x600).

Comment 2

[Dan Pernokis]

I switched to 800x600 - still a problem.

When I go to Manage Identities and select one at random, I get the MI window with no scroll bar.  The window extends below the bottom of the screen.  When I attempt to resize by pulling (just clicking, actually) on the top margin, the window jumps down about 1cm and a scroll bar appears, but the window still extends well below the bottom.  At this point, scrolling does NOT bring everything into view.

I have to resize again (by pulling down the top margin and then dragging the window up) to view the full MI window.  The scroll is a sub-window -- the OK/CANCEL buttons are now pinned to the bottom and permanently visible.  After that, everything seems to work fine.

Comment 3

[rsx11m]

Magnus, did you use Windows or Linux for testing? On KDE4, the window automatically resizes to fit the screen size, thus the problem isn't apparent given that the scrollbar appears.

I couldn't find an existing bug on this either, hence filed this as a follow-up.

Comment 4

[Magnus Melin [:mkmelin]]

Tested on Ubuntu 16.04 (and thunderbird trunk)

Comment 5

[Richard Marti (:Paenglab)]

Attachment: Win7-identities.png

This is how the identities window looks at opening on Win 7 with 800x600 resolution and the task bar hiding automatically.

Clicking on the window titlebar lets it jump down. Then make the window smaller from from top to down and the dragging it up let the Okay/Cancel button show and the window has the scrollbar.

So, it's possible to use the dialog on small screens (on Windows) but it's not easily accessible.

Comment 6

[rsx11m]

https://dxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/am-identity-edit.js#18 sets the width of the window to match the parent Account Manager window, but leaves the height to its (apparently undefined) default height, which fills the full panel height then to avoid scrolling. Thus, maybe just inheriting the height in the same way would resolve the issue?

Comment 7

[Dan Pernokis]

Re Comment 5:
>> ...it's possible to use the dialog on small screens (on Windows) but it's not easily accessible.

Exactly.  It's extremely tedious to do more than one or two.  Thankfully, the Account Settings window has been fixed -- that removes some of the tedium.

Re Comment 6:
Using same height as AM sounds fine to me.  I would think the same fix to Bug 507640 could apply here too -- if inheritance doesn't work.

Comment 8

[rsx11m]

(In reply to rsx11m from comment #6)
> identity-edit.js#18 sets the width of the window to match the parent Account
> Manager window, but leaves the height to its (apparently undefined) default
> height, which fills the full panel height then to avoid scrolling.

This doesn't actually work for me at all on Windows 7 and Thunderbird trunk. The width of the Manage Identites dialog is much larger than of the corresponding Account Manager window when opening (also with a new profile), thus isn't inherited as designed. Changing the code to an explicit pixel value doesn't do anything either, so I'm at a loss here what's happening.

Comment 9

[rsx11m]

That's testing with a "normal" screen size (i.e., larger than default AM size).

Comment 10

[Richard Marti (:Paenglab)]

Attachment: proposed fix

This patch makes the Identity dialog always the same height as the Account Mahager.

I haven't set it into the

if (accountDialog.documentElement.getAttribute("sizemode") == "normal") {}

because this only applies when the Account Manager size is changed. But this isn't the case when it's only opened on a small desktop.

Ace, is there a better solution?

Comment 11

[rsx11m]

(In reply to Richard Marti (:Paenglab) from comment #10)
> because this only applies when the Account Manager size is changed. But this
> isn't the case when it's only opened on a small desktop.

I'm wondering if, for reasons of consistency, the same should apply then for the width as well?

Comment 12

[Richard Marti (:Paenglab)]

Attachment: proposed fix round 2

This patch sets always the width and height of the Account Manager. Setting the width of the contentFrame after resizing the AM gave me almost always a too slim window for it's height which didn't looks so good.

Comment 13

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #10)
> I haven't set it into the
> 
> if (accountDialog.documentElement.getAttribute("sizemode") == "normal") {}
> 
> because this only applies when the Account Manager size is changed. But this
> isn't the case when it's only opened on a small desktop.
> 
> Ace, is there a better solution?

What do you mean here? What is the 'sizemode' on small desktop and why?

Comment 14

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #13)
> (In reply to Richard Marti (:Paenglab) from comment #10)
> > I haven't set it into the
> > 
> > if (accountDialog.documentElement.getAttribute("sizemode") == "normal") {}
> > 
> > because this only applies when the Account Manager size is changed. But this
> > isn't the case when it's only opened on a small desktop.
> > 
> > Ace, is there a better solution?
> 
> What do you mean here? What is the 'sizemode' on small desktop and why?

There is no 'sizemode' initially when opening on small screens. Because AM stores the dimensions I made the AM big on a bigger screen. Then I reduced the resolution to 800x600 and opened the AM again. Now it uses the whole screen but all window borders and buttons are available but there is still no 'sizemode'. The 'sizemode' appears only when I resize the AM window.

Comment 15

[:aceman]

Would it work for you if the condition was changed to sizemode != "maximized" ?

Comment 16

[Richard Marti (:Paenglab)]

Does this also work when no sizemode attribute exists? Never seen "maximized" in AM, so it would always be true. I don't see what's the difference between the sizemode != "maximized" statement and no statement.

Comment 17

[:aceman]

Yes I think when there is no attribute then getAttribute("sizemode") returns null and that != "maximized". I just do not want to set the sizes if the window is already maximized. If that condition can never happen, my suggested code still does no harm.

Comment 18

[Richard Marti (:Paenglab)]

Attachment: proposed fix round 3

Patch with sizemode != "maximized" condition.

Comment 19

[:aceman]

Comment on attachment 8799148 [details] [diff] [review]
proposed fix round 3

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

OK, it does work. But I see now it completely obscures the whole account manager window. I'm not sure we want that. Why was setting the width to the contentFrame only enough? Are there any elements that would wrap if the width was of the contentFrame (as before the patch) ?

Comment 20

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #19)
> Comment on attachment 8799148 [details] [diff] [review]
> proposed fix round 3
> 
> Review of attachment 8799148 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, it does work. But I see now it completely obscures the whole account
> manager window. I'm not sure we want that. Why was setting the width to the
> contentFrame only enough? Are there any elements that would wrap if the
> width was of the contentFrame (as before the patch) ?

As I wrote in comment 12, it's because the dialog is very slim (AM width minus the accounts tree) but has the same height, also without my patch.

Comment 21

[Richard Marti (:Paenglab)]

I could also make the width of the Identity dialog 100px smaller than the AM window.

Comment 22

[rsx11m]

I was only throwing in an idea here, whatever you guys think looks best.  :-)

Yes, they probably shouldn't have /exactly/ the same size, especially not if it opens aligned to the AM window rather than in an offset, so something inbetween as suggested in comment #21 should be fine.

Comment 23

[Dan Pernokis]

I agree it should not exactly overlap/cover up the AM window.  And it looks like there isn't anything that would truncate if it were narrower, unless the user has a very long address or folder names, or long signature lines.

Comment 24

[Richard Marti (:Paenglab)]

Attachment: proposed fix round 4

Same as round 3 but with a 100px less wide Identity window than the Account manager.

Comment 25

[:aceman]

Comment on attachment 8801523 [details] [diff] [review]
proposed fix round 4

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

Thanks, this is better. We just need to decide whether it should be absolute -100px or e.g. 90% of the AM.

Comment 26

[Richard Marti (:Paenglab)]

Attachment: proposed fix round 5

Patch with using 90% width.

Comment 27

[:aceman]

Comment on attachment 8801586 [details] [diff] [review]
proposed fix round 5

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

Works for me.

Comment 28

[Dan Pernokis]

And my preference is percentage size too.  Thanks, guys.

Comment 29

[Richard Marti (:Paenglab)]

https://hg.mozilla.org/comm-central/rev/9cecc12b0c9f64dcbe39e8596507030e426aed3b

Comment 30

[Richard Marti (:Paenglab)]

Comment on attachment 8801586 [details] [diff] [review]
proposed fix round 5

[Approval Request Comment]
User impact if declined: Identity dialog not fully accessible on small screens
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low, small changes

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 8801586 [details] [diff] [review]
proposed fix round 5

(In reply to Richard Marti (:Paenglab) from comment #30)
> Risk to taking this patch (and alternatives if risky): low, small changes
I can crash the entire system if I only change one single character in the right spot ;-) So small doesn't equate to low risk.

The right argument would be that there are small changes in a code path that is not so frequently used.

That said, you missed TB 50 beta 2, and I doubt that we will do a beta 3. So it will go into TB 51 beta (via Aurora). If I didn't miscount, that could then go into TB 45.6 if Kent is so inclined.

Comment 32

[Jorg K (CEST = GMT+2)]

Oops, C-C got landed without the reviewer in the commit message. Looks like it was r=aceman.

Aurora (TB 51):
https://hg.mozilla.org/releases/comm-aurora/rev/5fd3dfd960f14cc8b7bab32b79990566515850dc

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 8801586 [details] [diff] [review]
proposed fix round 5

Beta (TB 50):
https://hg.mozilla.org/releases/comm-beta/rev/91c110a38bf4dade626da353f98243156c396fdd

The patch didn't apply cleanly, two hunks failed. This was to some overzealous 'boy-scout'-removing-trailing-spaces clean-up :-( - I just left those hunks out.

Usually I'm in favour of clean-up but this example shows that it can cause trouble.

Comment 34

[Kent James (:rkent)]

Comment on attachment 8801586 [details] [diff] [review]
proposed fix round 5

https://hg.mozilla.org/releases/comm-esr45/rev/c800630b56b3