Closed Bug 714186 Opened 13 years ago Closed 12 years ago

Add padding to top of windows when fullscreen on Lion

Categories

(Firefox :: Theme, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 16

People

(Reporter: zpao, Assigned: zpao)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

Attached image what crampy mcCramperson looks like (deleted) —
I put this in toolkit > themes because if we make any other windows fullscreen capable, then this applies. Feel free to disagree and move to firefox... We should probably have more padding at the top of the window when in Lion full screen. It feels cramped up there. See attachment for how it's looking with (with WIP bug 639705).
On top of looking cramped, I'm constantly triggering the menubar dropdown trying to click tabs.
After talking with shorlander, I'm going to go with an additional 10-11px. Changing the height of the ::before pseudoelement here (https://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser.css#1916) does the trick, but applying changes via CSS with a sizemode="fullscreen" rule is rather slow... there's a noticeable delay after the transition is complete before the rules apply (I think sizemode isn't updated synchronously). So what I'm thinking is inserting a new toolbar from browser.js which we can style directly (or use a ::before on that if needed). That way it happens immediately. The non-Lion FS code does something similar (adds a toolbar to catch mouse events for showing the main toolbar) Another option would be to add/remove a new attribute to the window following the fullscreen event which we could use for styling. AFAIK there's no way to modify pseudoelements from JS, so changing the style of the existing one doesn't seem to be an option.
Assignee: nobody → paul
Component: Themes → Theme
Product: Toolkit → Firefox
QA Contact: themes → theme
(In reply to Paul O'Shannessy [:zpao] from comment #2) > Another option would be to add/remove a new attribute to the window > following the fullscreen event which we could use for styling. And actually "inFullscreen" already exists https://mxr.mozilla.org/mozilla-central/search?string=inFullscreen&find=css&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central - I just need actually set it on Lion. It's still not smooth though - since it's happening after the fullscreen event, the transition is already complete, then the extra height is added (at least when entering, things seem ok when exiting... weird). A possible solution to this (and other related FS choppyness) is if we add a couple more events, similar to how the Cocoa APIs work. It has a "before" event, a "failed" event, and a "complete" event. Cocoa also differentiates "event" vs "exit". That's all fodder for some other bug but it's on my brain here...
Bah, so sometimes it's smooth, sometimes it's not, it depends on how the toggle happens (our menu = smooth vs native controls = not). I filed bug 739749 for more FS events which we could use to solve this sometime in the future.
Attached patch Patch v0.1 (obsolete) (deleted) — Splinter Review
The change here is based on the numbers Stephen gave me. This only addresses tabs on top, tabs on bottom requires a bit more work I and I couldn't get the colors looking right (toolbar looked too dark), amongst other issues. We could use a transition for the height and make this look a little smoother, but we only want it when activating natively. If we use that and trigger FS ourselves, we're choppy the other way. Can't win either way.
Attachment #612406 - Flags: ui-review?(shorlander)
Attachment #612406 - Flags: feedback?(dao)
Comment on attachment 612406 [details] [diff] [review] Patch v0.1 This looks like it will behave quite annoyingly when previewing a persona in fullscreen mode.
Attachment #612406 - Flags: feedback?(dao) → feedback-
Attached patch Patch v0.2 (obsolete) (deleted) — Splinter Review
What about this then? It also addresses my concerns from bug 716450 about the bg image shifting in the transition.
Attachment #612406 - Attachment is obsolete: true
Attachment #612406 - Flags: ui-review?(shorlander)
Attachment #612677 - Flags: feedback?(dao)
Attachment #612677 - Flags: feedback?(dao) → review?(dao)
Dão, any chance you could look at this sooner rather than later? I'd like to finish up the fullscreen loose ends as much as possible.
Comment on attachment 612677 [details] [diff] [review] Patch v0.2 This patch doesn't apply cleanly to mozilla-central. Do I need to apply another patch first?
Attached patch Patch v0.2 (unbitrotted) (deleted) — Splinter Review
Sorry, it bitrotted when we split browser.js up.
Attachment #612677 - Attachment is obsolete: true
Attachment #612677 - Flags: review?(fryn)
Attachment #612677 - Flags: review?(dao)
Attachment #633821 - Flags: review?(fryn)
Attachment #633821 - Flags: review?(fryn) → review+
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 16
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
D: Why are we reducing screen state? Isn't the point of fullscreen to have all the space you can get for the content? I agree the previous style was a bit too cramped, but all that padding feels like a big waste of space.
I agree with Reuben Morais I don't know about Chrome but Opera doesn't use padding at the top and it runs just fine I am not annoyed by the top bar each time I want to click a tab I have opened.
Padding is the wrong way to do this anyway on OSX 10.7+ the menubar should push down the window chrome which is then still clickable if you overshot as it's not covered up. Just look at how safari does it. I'd throw in parity safari on this as well. I personally find the way FF, Opera, and Chrome handle the menubar in fullscreen mode incredibly annoying.
Depends on: 944228
Depends on: 1130209
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: