Closed Bug 596151 Opened 9 years ago Closed 9 years ago

when in touch-screen mode app tabs aren't as tall as real tabs, therefore self-conscious

Categories

(Firefox :: Theme, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b8

People

(Reporter: shaver, Assigned: Felipe)

Details

Attachments

(2 files, 2 obsolete files)

I am on a laptop with a touch screen, and that means I get extra big tabs for use with my dialing wand.  My app tabs are late bloomers.

http://grab.by/grabs/ff70ee70495b94dad9714876f1b0762c.png
Thanks for filing Shaver, I have a WIP patch but didn't file a bug about it yet.

The problem is that the min-height touch-enabled style is in the .tabbrowser-tabs element, but as the app tabs are position:fixed they don't stretch to the whole height of the container.
Also, the tabs are taller than they should be due to some recent theme changes.
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Some similar reasons here for using -moz-calc as there was in bug 579683 (need to account for the height represented by the padding and borders, currently 7px)

The 2px adjustment are due to some recent unwanted increases in that height, probably fallout from various theme tweaking changes (e.g. moving the border from the TabsToolbar to the navigator-toolbox).
With the -2px in the calc the dimensions go back as implemented in bug 572472
Attachment #479702 - Flags: review?(dao)
Comment on attachment 479702 [details] [diff] [review]
Patch v1

> .tabbrowser-tabs:-moz-system-metric(touch-enabled) {
>-  min-height: 7.3mozmm;
>+  min-height: -moz-calc(7.3mozmm - 2px);
> }

I guess one extra pixel is due to DirectWrite -- we need to fix that globally, not just for touch-enabled.

(In reply to comment #1)
> The problem is that the min-height touch-enabled style is in the
> .tabbrowser-tabs element, but as the app tabs are position:fixed they don't
> stretch to the whole height of the container.

So how about always setting the min-height on the tab elements, e.g. .tabbrowser-tab or .tab-content?
Attachment #479702 - Flags: review?(dao) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
(In reply to comment #3)
> So how about always setting the min-height on the tab elements, e.g.
> .tabbrowser-tab or .tab-content?

The min-height on .tabbrowser-tab produced odd results like a 1px height difference between a single tab and two or more tabs, but .tab-content worked perfectly.

 The size calculations is the same as the previous patch. I've  verified that it results in the same height with DirectWrite on and off.


Also moved the declaration from the "Tab-strip" section to the "Tab" section inside the file
Attachment #479702 - Attachment is obsolete: true
Attachment #486956 - Flags: review?(dao)
(I thought this was already marked as a blocker)
blocking2.0: --- → ?
Comment on attachment 486956 [details] [diff] [review]
Patch v2

>+.tab-content:-moz-system-metric(touch-enabled) {
>+  min-height: -moz-calc(7.3mozmm - 2px - 7px); /* subtract existing borders and paddings from the height. */
>+}

What padding?
I added "borders and paddings" to the comment more as a generic annotation (i.e. if some changes in borders or paddings happen in the future this might need to be adjusted).

I don't know if atm. all is just borders or if there are any paddings at play here (I couldn't find any in DOMi), but I believe that 1px of the fine tuning here was needed due to the margin/padding change from bug 590873.
DOMi reports a 4px top and 3px bottom border -- what are the other two pixels? If we don't actually want tabs to be 7.3 mm tall, we shouldn't subtract some pixels but use a lower mm value.
Attached patch Patch v3Splinter Review
Ok yeah, I incorporated the -2px in the mm value now. I was keeping the 7.3mm to keep the original measurements but it's better to have the final value wanted there.
Attachment #486956 - Attachment is obsolete: true
Attachment #491358 - Flags: review?(dao)
Attachment #486956 - Flags: review?(dao)
Comment on attachment 491358 [details] [diff] [review]
Patch v3

>+.tab-content:-moz-system-metric(touch-enabled) {
>+  min-height: -moz-calc(6.8mozmm - 7px); /* subtract existing borders from the height. */
>+}

nit: "subtract borders from the desired height"

There's a @media all and (-moz-touch-enabled) section some lines below. Can you move this there and drop :-moz-system-metric(touch-enabled)?
Attachment #491358 - Flags: review?(dao) → review+
Attached patch Patch to landSplinter Review
Attachment #491640 - Flags: review+
Attachment #491640 - Flags: approval2.0?
Attachment #491640 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/1709bbd7964f
Status: ASSIGNED → RESOLVED
blocking2.0: ? → ---
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
You need to log in before you can comment on or make changes to this bug.