Closed Bug 1307773 Opened 4 years ago Closed 4 years ago

Manage Identities window is too tall to fit monitor screen

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set
normal

Tracking

(thunderbird49 wontfix, thunderbird50 fixed, thunderbird_esr4550+ fixed, thunderbird51 fixed, thunderbird52 fixed)

RESOLVED FIXED
Thunderbird 52.0
Tracking Status
thunderbird49 --- wontfix
thunderbird50 --- fixed
thunderbird_esr45 50+ fixed
thunderbird51 --- fixed
thunderbird52 --- fixed

People

(Reporter: rsx11m.pub, Assigned: Paenglab)

References

()

Details

Attachments

(2 files, 4 obsolete files)

+++ 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.)
That was a separate bug (can't find it now), but at least for me a scrollbar appears (tried 800x600).
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.
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.
Tested on Ubuntu 16.04 (and thunderbird trunk)
Attached image 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.
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?
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.
(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.
That's testing with a "normal" screen size (i.e., larger than default AM size).
Attached patch proposed fix (obsolete) — Splinter Review
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?
Attachment #8798648 - Flags: feedback?(acelists)
(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?
Attached patch proposed fix round 2 (obsolete) — Splinter Review
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.
Assignee: nobody → richard.marti
Attachment #8798648 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8798648 - Flags: feedback?(acelists)
Attachment #8799104 - Flags: review?(acelists)
(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?
(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.
Would it work for you if the condition was changed to sizemode != "maximized" ?
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.
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.
Attached patch proposed fix round 3 (obsolete) — Splinter Review
Patch with sizemode != "maximized" condition.
Attachment #8799104 - Attachment is obsolete: true
Attachment #8799104 - Flags: review?(acelists)
Attachment #8799148 - Flags: review?(acelists)
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) ?
Attachment #8799148 - Flags: feedback?(mkmelin+mozilla)
Attachment #8799148 - Flags: feedback?(iann_bugzilla)
(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.
I could also make the width of the Identity dialog 100px smaller than the AM window.
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.
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.
Attached patch proposed fix round 4 (obsolete) — Splinter Review
Same as round 3 but with a 100px less wide Identity window than the Account manager.
Attachment #8799148 - Attachment is obsolete: true
Attachment #8799148 - Flags: review?(acelists)
Attachment #8799148 - Flags: feedback?(mkmelin+mozilla)
Attachment #8799148 - Flags: feedback?(iann_bugzilla)
Attachment #8801523 - Flags: review?(acelists)
Attachment #8801523 - Flags: feedback?(mkmelin+mozilla)
Attachment #8801523 - Flags: feedback?(iann_bugzilla)
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.
Attachment #8801523 - Flags: review?(acelists) → feedback+
Patch with using 90% width.
Attachment #8801523 - Attachment is obsolete: true
Attachment #8801523 - Flags: feedback?(mkmelin+mozilla)
Attachment #8801523 - Flags: feedback?(iann_bugzilla)
Attachment #8801586 - Flags: review?(acelists)
Comment on attachment 8801586 [details] [diff] [review]
proposed fix round 5

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

Works for me.
Attachment #8801586 - Flags: review?(acelists) → review+
And my preference is percentage size too.  Thanks, guys.
https://hg.mozilla.org/comm-central/rev/9cecc12b0c9f64dcbe39e8596507030e426aed3b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
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
Attachment #8801586 - Flags: approval-comm-esr45?
Attachment #8801586 - Flags: approval-comm-beta?
Attachment #8801586 - Flags: approval-comm-aurora?
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.
Attachment #8801586 - Flags: approval-comm-aurora? → approval-comm-aurora+
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 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.
Attachment #8801586 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8801586 [details] [diff] [review]
proposed fix round 5

https://hg.mozilla.org/releases/comm-esr45/rev/c800630b56b3
Attachment #8801586 - Flags: approval-comm-esr45? → approval-comm-esr45+
You need to log in before you can comment on or make changes to this bug.