Closed Bug 966655 Opened 10 years ago Closed 10 years ago

Scrollbar shown for recipient list when empty (involving screen dpi scaling): slightly insufficient default height of msgHeadersToolbar cripples visibility of recipients rows (see Bug 1056404)

Categories

(Thunderbird :: Theme, defect)

x86_64
Windows 8.1
defect
Not set
major

Tracking

(thunderbird33 fixed, thunderbird34 fixed, thunderbird_esr3132+ fixed)

RESOLVED FIXED
Thunderbird 34.0
Tracking Status
thunderbird33 --- fixed
thunderbird34 --- fixed
thunderbird_esr31 32+ fixed

People

(Reporter: aryx, Assigned: thomas8)

References

Details

(Whiteboard: [major because of extremely annoying/exposed UX for affected users])

Attachments

(2 files, 1 obsolete file)

Windows 8.1 Pro 64 bit, Daily 20140201

The compose window's recipient list has a scrollbar even if it is empty. Likely fallout from bug 960672.
Hmm, I'm seeing no scrollbar, even on my Win8 with 100% scaling and also on Win8 with 135% scaling. Have you tried TB in safe mode? Please can you add a screenshot?
Attached image screenshot of issue
Windows scaling is set to manually use 125%.
I was wrong in comment 1 with 135% on one machine, it is 120%. Everything bigger than 120% shows the scrollbar. I tried also a build from 2.1.14 (20140102222551) and it also happens with the old styling.
Also tb24, maybe from bug 425451?
Blocks: 1056404
(In reply to Magnus Melin from comment #4)
> Also tb24, maybe from bug 425451?

I don't think this comes from bug 425451, but it might, as we removed a fixed minimum hight of 132 px from css and replaced that with calculated height in patch attachment 766653 [details] [diff] [review].
But I don't see this problem on TB 24.6 / WinXP, even with 150% OS dpi scaling.

Fortunately, the fix for this should be easy:

I'm running at 150% scaling (144dpi), and the superfluuous scroll bar goes away by dragging the recipient area just a few pixels bigger vertically; so all we need to do is to add those extra pixels to the default size of whatever widget is appropriate. I remember Richard fine-tuned the vertical pixel sizes, so he'll know.
Otherwise we could even do that in code calculations:

Attachment 766653 [details] [diff]
+  // Set minimum number of rows shown for address widget, per hardwired
+  // rows="1" attribute of addressingWidget, to prevent resizing the
+  // subject and format toolbar over the address widget.
+  // This lets users shrink the address widget to one row (with delicate UX)
+  // and thus maximize the space available for composition body,
+  // especially on small screens.
+  msgHeadersToolbar.minHeight = msgHeadersToolbar.boxObject.height;
+
+  // Set default number of rows shown for address widget.
+  addressingWidget.setAttribute("rows", awNumRowsShownDefault);
+  msgHeadersToolbar.height = msgHeadersToolbar.boxObject.height;
+
+  // Update addressingWidget internals.
+  awCreateOrRemoveDummyRows();

I'm also wondering how Richard's vertical pixels finetuning or my patch interacts with 
awCreateOrRemoveDummyRows() function:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/addressingWidgetOverlay.js#856

Unfortunately, the effect of this bug isn't minor, as described in Bug 1056404: Effectively, in the absence of manual user intervention for each and every composed message to drag the recipient area bigger (which is an inacceptable hassle in the long run), this overrides mail.compose.addresswidget.numRowsShownDefault=3; by reducing visible recipient rows to 2. Even for an advanced user like me, the UX of this is really unstable, insecure and odd because without manual scrolling around, you only ever get to see the last recipient which you entered and all others disappear as soon as you press ENTER. I keep asking myself what happened to the first recipient after entering the second, while there's still free space for a third. Seeing the just-entered recipient AND the last-but-one recipient feels a lot better/safer in the odd circumstances of the traditional vertical recipient list which is awaiting redemption from Bug 440377.

So this should be fixed asap and uplifted to go into 31esr or whatever the nearest available update to 31. Recipient area is enough of a headache with the clumsy vertical design and autocomplete issues, we really shouldn't break everything in there at the same time.

And fwiw, seeing how much users react to everything we touch in recipient area, I repeat my call to expose mail.compose.addresswidget.numRowsShownDefault=3; in the Options UI, as it's one of the most crucial parts of the UI, and different users have different needs. Think of that company which always cc's the CEO, his vice and the secretary or whomsoever...
As explained in comment 5, marking major to keep on the urgent radar because this cripples the addressing widget to only 1 filled and 1 empty row per Bug 1056404 (instead of default 3 rows), which is very poor UX in addition to other issues currently seen in recipient area.
Severity: trivial → major
(In reply to Richard Marti (:Paenglab) from comment #3)
> I was wrong in comment 1 with 135% on one machine, it is 120%. Everything
> bigger than 120% shows the scrollbar. I tried also a build from 2.1.14
> (20140102222551) and it also happens with the old styling.

Fwiw, on WinXP, TB31, 1920 x 1080, 120% scaling (115dpi) or even or even 125% (120dpi), I do NOT yet get the scrollbars (but I get them with 130%, tired of testing further).

But even with 500% scaling, which is the maximum for my setup, it still takes only around the equivalent of original 2px extra vertical height to make the scrollbar disappear. Which imo supports my theory that just adding a fex pixels fixed extra height somewhere in the header area will fix this issue for everyone.
Have you tested how a pre bug 425451 version works with scaling. Maybe it's the same but to this times it wasn't tested because of the lack of such high res monitors as we have today.
I think this patch is the most simple way of working around this issue, which should work across all systems and themes (and no side effects even if it wouldn't).

Richard, can you test if this works for you on Win8, say with 100%, 130%, 500% screen resolution dpi scaling? Tests on other OS, Linux, MAC with different scalings also welcome...anyone?

Basically, the undesired scroll bar goes away when user drags down the header area border by 1 or 2 pixels, which then provides sufficient vertical space for msgHeadersToolbar to fully fit the default number of recipient rows. So this patch does the same programatically. This moves the initial position of subject line down by 2px which is unnoticeable to the eye; according to my screen ruler, the distance was 1px smaller than distance between recipient lines anyway (just 1px added via DOMi does the trick for me on Win XP even with 500% screen dpi scaling, but following the old addon code attachment 703422 [details], let's add 2px anyway to be on the safe side).

I largely agree with Richard's analysis but I think the UX of this per bug 1056404 is bad enough to demand an immediate pragmatic solution without wasting unpredictable amounts of time on finding the real causes, which might be bugs in XUL, rounding errors, peculiarities of box object height definitions, wrong height calculation assumptions on our side, functions in our code changing heights after the fact, etc.

Some random candidates for causes (incomplete):

1) Box object height definition (used in TB's calculation)

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Box_Objects#Retrieving_Position_and_Size
> The width and height properties are also measured in pixels and return the
> width and height of the element, including CSS padding and borders.
> CSS margins are excluded (!).

2) XUL bugs like these (not all or none of these might apply, didn't check details)
Bug 491788 - XUL listbox sometimes has unnecessary scrollbar
Bug 476048 - Wizard vertical scrollbar displayed - appears to miscalculate available height (due to buttons?)

(In reply to Richard Marti (:Paenglab) from bug 1056404 comment #4)
> This looks like a rounding issue. On XP Classic I get quite good results
> with setting the border width to 2pt instead 2px (but not when scaling is
> 100%). On 125% the 2px borders are calculated in DONi with 1.6px and the 2pt
> borders with 2px. On XP normal the border change doesn't work but it works
> with setting a height for addressingWidgetItem and dummy-row of 25px (but
> again not always). On Win8 it works for scaling >=120% with a height of
> 2.15em.
> 
> Because it isn't the same on all OS versions it's very hard to clearly
> nominate a solution and also why it is failing. Maybe it is also a toolkit
> issue but we need a reduced testcase for this.
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #8478924 - Flags: ui-review?(richard.marti)
Attachment #8478924 - Flags: review?(richard.marti)
Comment on attachment 8478924 [details] [diff] [review]
Patch v.1: Add 2px extraHeight to msgHeadersToolbar

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

LGTM and I think the JS is easy enough I can review it. r+ with the comment addressed.

Thank you, Thomas

(In reply to Thomas D. from comment #9)
> Created attachment 8478924 [details] [diff] [review]
> Patch v.1: Add 2px extraHeight to msgHeadersToolbar
> 
> Basically, the undesired scroll bar goes away when user drags down the
> header area border by 1 or 2 pixels, which then provides sufficient vertical
> space for msgHeadersToolbar to fully fit the default number of recipient
> rows. So this patch does the same programatically. This moves the initial
> position of subject line down by 2px which is unnoticeable to the eye;

I see a difference and thinking about filing a followup bug for this. :)

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +52,5 @@
>    let msgHeadersToolbar = document.getElementById("MsgHeadersToolbar");
>    let addressingWidget = document.getElementById("addressingWidget");
>    let awNumRowsShownDefault =
>      Services.prefs.getIntPref("mail.compose.addresswidget.numRowsShownDefault");
> +  // Work around bug 966655: extraHeight 2 pixels for msgHeadersToolbar ensures

Please a empty line before the comment to make the code better readable.
Attachment #8478924 - Flags: ui-review?(richard.marti)
Attachment #8478924 - Flags: ui-review+
Attachment #8478924 - Flags: review?(richard.marti)
Attachment #8478924 - Flags: review+
nitfixed patch

ui-r+,r+=paenglab from comment 10
Attachment #8478924 - Attachment is obsolete: true
Attachment #8480093 - Flags: ui-review+
Attachment #8480093 - Flags: review+
Keywords: checkin-needed
Comment on attachment 8480093 [details] [diff] [review]
Patch v.1b: Add 2px extraHeight to msgHeadersToolbar

[Approval Request Comment]
Regression caused by (bug #): unknown (scaled screen resolutions weren't tested)
User impact if declined: crippling the default recipient area to 2 visible rows instead of 3, while still consuming the space for 3 rows, which is extremely odd UX in one of the most used parts of the UI (dogfood)
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): none (this increases the user-adjustable default height of msgHeaderToolbar by 2px)
Attachment #8480093 - Flags: approval-comm-esr31?
Attachment #8480093 - Flags: approval-comm-beta?
Attachment #8480093 - Flags: approval-comm-aurora?
Attachment #8480093 - Flags: approval-comm-aurora? → approval-comm-aurora+
https://hg.mozilla.org/comm-central/rev/b18d8a612e53
https://hg.mozilla.org/releases/comm-aurora/rev/8145b442b391
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 34.0
Attachment #8480093 - Flags: approval-comm-esr31?
Attachment #8480093 - Flags: approval-comm-esr31+
Attachment #8480093 - Flags: approval-comm-beta?
Attachment #8480093 - Flags: approval-comm-beta+
Mark, thanks for rapid response, landing and uplifting of this patch. :)

Although trivial in cause (2px vertical height missing depending on circumstances), this bug makes for extremely annoying UX for the everyday scenario of adding recipients in composition, as it reduces the default number of visible recipients from the already low default of 2 (plus 1 empty row) to only one (plus 1 empty row), while still wasting the screen real estate for 3 rows, which imho makes TB look and behave really ridiculous (apart from showing the scrollbar where there's nothing to scroll). As big screens with resolution dpi scaling are more frequent these days (both private and business environments), a large number of users will be affected by this. The patch is absolutely zero risk, a one-liner which just adds the 2px to the header height.
Summary: Scrollbar shown for recipient list when empty → Scrollbar shown for recipient list when empty (involving screen dpi scaling): slightly insufficient default height of msgHeadersToolbar cripples visibility of recipients rows (see Bug 1056404)
Whiteboard: [major because of extremely annoying/exposed UX for affected users]
You need to log in before you can comment on or make changes to this bug.