Update Lion Fullscreen window styling offsets for OS X tabs in the titlebar

VERIFIED FIXED in Firefox 29

Status

()

defect
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

Trunk
Firefox 31
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified, firefox31 verified)

Details

(Whiteboard: [Australis:P4], )

Attachments

(1 attachment, 1 obsolete attachment)

Update the calculations from bug 714186 now that we have tabs in the titlebar otherwise the LWT header image shifts after the animation while entering fullscreen. See the code comment for more info.

On a related note, I noticed that the space above tabs differs depending on whether a LWT is applied. Perhaps this bug can standardize that?
(In reply to Matthew N. [:MattN] from comment #0)
> On a related note, I noticed that the space above tabs differs depending on
> whether a LWT is applied.

For the record, this was now filed as bug 879607.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Whiteboard: [Australis:M?] → [Australis:M7]
Removing the items from M7 that do not block us from landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P4]
Before you can adjust any offsets, first make sure there are offsets to adjust.
First bug 939010 needs to land, which re-introduces padding for normal windows as a side effect, and then  bug 944229 needs to fix PB windows.
Depends on: 944229, 939010
This properly calculates the offsets, so that the tab bar does not jump around vertically anymore (without, it will jump by 1px currently).

This also removed the need to offset the lwtheme background.
Assignee: MattN+bmo → maierman
Attachment #8344393 - Flags: review?(MattN+bmo)
Comment on attachment 8344393 [details] [diff] [review]
Update Lion Fullscreen window styling offsets for OS X.

Without extra context, it's not clear to me what tabOffset exactly means. Can you come up with a more specific name?
Flags: needinfo?(maierman)
Whiteboard: [Australis:M?][Australis:P4] → [Australis:M?][Australis:P4] [feature] p=0
Comment on attachment 8344393 [details] [diff] [review]
Update Lion Fullscreen window styling offsets for OS X.

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

I was waiting for you to address Dão's comment btw.

::: browser/themes/osx/browser.css
@@ +8,5 @@
>  %filter substitution
>  %define forwardTransitionLength 150ms
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
>  %define conditionalForwardWithUrlbarWidth 27
> +%define tabOffset 1px

Nit: move this between spaceAboveTabbar and toolbarButtonPressed to keep this alphabetical (ignoring forwardTransitionLength).

I unfortunately don't have a better suggestion for the name of the define so I could live with this name.
Attachment #8344393 - Flags: review?(MattN+bmo) → review+
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis:M?][Australis:P4] [feature] p=0 → [Australis:M?][Australis:P4]
Sorry for the delay. Real life kept me busy.

I don't have a good alternative for the name either... This was already the best I could come up with...
I'll probably have time next week to address your review, so unless somebody comes up with a better name before then, I'll just keep tabOffset.
Flags: needinfo?(maierman)
Sounds like this is done, except for a nitfix?
Flags: needinfo?(MattN+bmo)
Nils or Matt, can you describe what tabOffset is supposed to mean? I can try to come up with a better name then.
Whiteboard: [Australis:M?][Australis:P4] → [Australis:P4]
Duplicate of this bug: 986449
While trying to come up with a better name for the "tabOffset" define in attachment 8344393 [details] [diff] [review], I needed to figure out what it all affected. As a simple check, I set it to really large values such as 100px and it had no effect in fullscreen (native or DOM) with or without a theme AFAICT. This avoid the naming problem too.

I measured the height above the tabs and it seems the same to me without the extra one pixels. The only difference is that there is a border on the top of the window when not in fullscreen but the transition seems the same for me with or without the 1px. Do you both see the same?
Attachment #8395088 - Flags: review?(timdream)
Attachment #8395088 - Flags: review?(maierman)
Flags: needinfo?(MattN+bmo)
Comment on attachment 8395088 [details] [diff] [review]
Option B - Remove @tabOffset@

I don't build Firefox on Mac, will try this out after I build it.
Comment on attachment 8395088 [details] [diff] [review]
Option B - Remove @tabOffset@

I am unable to apply this patch on m-c. Do I need to build with any specific branch?
Flags: needinfo?(MattN+bmo)
Comment on attachment 8395088 [details] [diff] [review]
Option B - Remove @tabOffset@

Never mind, there were some mix-up in my tree.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8395088 [details] [diff] [review]
Option B - Remove @tabOffset@

I can confirm this patch fixes bug 986449 but not bug 963590, nor bug 986796 and bug 986797 (privacy browsing window).
Attachment #8395088 - Flags: review?(timdream) → review+
Comment on attachment 8395088 [details] [diff] [review]
Option B - Remove @tabOffset@

https://hg.mozilla.org/integration/fx-team/rev/d97d25f58092

Since I didn't hear from Nils, I went ahead and landed my version as it seems like an improvement to me both in terms of what the user sees and in code-cleanliness (by using @spaceAboveTabbar@). A follow-up bug can be filed if I missed something.
Attachment #8395088 - Flags: review?(maierman)
Assignee: maierman → MattN+bmo
Whiteboard: [Australis:P4] → [Australis:P4][fixed-in-fx-team]
Attachment #8344393 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d97d25f58092
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Target Milestone: --- → Firefox 31
Comment on attachment 8395088 [details] [diff] [review]
Option B - Remove @tabOffset@

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 625989 
User impact if declined: The space above the tabs on OS X can vary based on whether a theme is applied or whether the user is in fullscreen.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low risk, could possibly just cause the tabstrip to offset the wrong amount if there was some case missed.
String or IDL/UUID changes made by this patch: None
Attachment #8395088 - Flags: approval-mozilla-beta?
Attachment #8395088 - Flags: approval-mozilla-aurora?
Attachment #8395088 - Flags: approval-mozilla-beta?
Attachment #8395088 - Flags: approval-mozilla-beta+
Attachment #8395088 - Flags: approval-mozilla-aurora?
Attachment #8395088 - Flags: approval-mozilla-aurora+
Verified as fixed using the following environment:
FF 27.0b6
Build Id: 20140407135746
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:29.0) Gecko/20100101 Firefox/29.0
Verified as fixed using the following environment:
FF 29.0b6
Build Id: 20140407135746
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:29.0) Gecko/20100101 Firefox/29.0
Verified as fixed with Fx 30 beta 7 (Build ID: 20140522105902) on Mac OS X 10.7.5:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:30.0) Gecko/20100101 Firefox/30.0
Verified as fixed using the following environment:

FF 31.0b3
Build Id: 20140619131915
OS: Mac Os X 10.7.5
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.