Closed Bug 1168397 Opened 6 years ago Closed 6 years ago

Bookmarks Toolbar disappears in OS X Fullscreen

Categories

(Firefox :: Toolbars and Customization, defect)

Unspecified
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: designakt, Assigned: xidorn)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

When clicking the green fullscreen icon in OS X the Bookmarks Toolbar disappears. Reappears when leaving fullscreen again.
OS: Unspecified → Mac OS X
Xidorn, any chance this is a result of one of your changes to mac fullscreen?
Flags: needinfo?(quanxunzhen)
I guess it could be regression of bug 1162752 or bug 1165570.

It is also possible that it is something "fixed" by one of the bugs, instead of a regression. The questions are: Do we display the bookmark toolbar in fullscreen mode in platforms other than Mac? Dose Safari do that?
Flags: needinfo?(quanxunzhen)
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #2)
> Do we display the bookmark toolbar in fullscreen mode in platforms other than Mac? 
The mac fullscreen is not a fullscreen to watch video in, but a way to maximize the area an app can use. So there is no equivalent in windows so far to compare it to. So far we displayed all UI the same as in normal when in mac fullscreen. But now we just hide the Bookmarks Toolbar.

> Dose Safari do that?
Yes
(In reply to Markus Jaritz [:maritz] (UX) from comment #3)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #2)
> > Do we display the bookmark toolbar in fullscreen mode in platforms other than Mac? 
> The mac fullscreen is not a fullscreen to watch video in, but a way to
> maximize the area an app can use. So there is no equivalent in windows so
> far to compare it to. So far we displayed all UI the same as in normal when
> in mac fullscreen. But now we just hide the Bookmarks Toolbar.

You probably confused the fullscreen mode with DOM Fullscreen API.

The DOM Fullscreen API is a web feature with which web content can make itself fullscreen for playing videos, games, etc. It is available on all systems with (almost) the same experience that all UI elements from browser are hidden.

The fullscreen mode is a browser feature which allows user to maximize the area of the web page. It is also available on all systems, not only Mac.

Now I'm pretty sure that this is caused by bug 1162752 where I made fullscreen mode on Mac share more code with other platforms. And I read a comment which says the common code is to "show/hide menubars, toolbars (except the full screen toolbar)", so I assume it is the behavior we do in other platforms for the fullscreen mode.

But, anyway, fixing this shouldn't be very hard. I'm not keen to fix it, and think it's fine either way. :Gijs, could you see which is what we want and fix it or close the bug accordingly?

> > Dose Safari do that?
> Yes

Hmm, Safari does it far better. They hide the bookmarks and tabbar by default and only display them when you hover the top. We probably want something like that.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #4)
> (In reply to Markus Jaritz [:maritz] (UX) from comment #3)
> > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #2)
> > > Do we display the bookmark toolbar in fullscreen mode in platforms other than Mac? 
> > The mac fullscreen is not a fullscreen to watch video in, but a way to
> > maximize the area an app can use. So there is no equivalent in windows so
> > far to compare it to. So far we displayed all UI the same as in normal when
> > in mac fullscreen. But now we just hide the Bookmarks Toolbar.
> 
> You probably confused the fullscreen mode with DOM Fullscreen API.
> 
> The DOM Fullscreen API is a web feature with which web content can make
> itself fullscreen for playing videos, games, etc. It is available on all
> systems with (almost) the same experience that all UI elements from browser
> are hidden.
> 
> The fullscreen mode is a browser feature which allows user to maximize the
> area of the web page. It is also available on all systems, not only Mac.
> 
> Now I'm pretty sure that this is caused by bug 1162752 where I made
> fullscreen mode on Mac share more code with other platforms. And I read a
> comment which says the common code is to "show/hide menubars, toolbars
> (except the full screen toolbar)", so I assume it is the behavior we do in
> other platforms for the fullscreen mode.
> 
> But, anyway, fixing this shouldn't be very hard. I'm not keen to fix it, and
> think it's fine either way. :Gijs, could you see which is what we want and
> fix it or close the bug accordingly?

I'm an engineer and this is a UX question. Markus is probably a better person to ask. :-)

Let's pull in shorlander and phlsa see how they feel, but personally I would be inclined to follow Safari on OS X, esp. as we should already have code that does this because it's what we do on Windows.

Also, Markus, what do you mean by "the green fullscreen icon" ? As Xidorn says, there's a distinction between DOM fullscreen and app fullscreen... but I've never seen a green fullscreen icon, so I'm confused what the exact steps to reproduce are. :-)

> > > Dose Safari do that?
> > Yes
> 
> Hmm, Safari does it far better. They hide the bookmarks and tabbar by
> default and only display them when you hover the top. We probably want
> something like that.

This is what we already do on Windows/Linux... there's a pref to turn it off (with a checkmark in the context menu), but otherwise, that's what we do.
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
Flags: needinfo?(mjaritz)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #5)
> I'm an engineer and this is a UX question. Markus is probably a better
> person to ask. :-)
> 
> Let's pull in shorlander and phlsa see how they feel, but personally I would
> be inclined to follow Safari on OS X, esp. as we should already have code
> that does this because it's what we do on Windows.
> 
> Also, Markus, what do you mean by "the green fullscreen icon" ? As Xidorn
> says, there's a distinction between DOM fullscreen and app fullscreen... but
> I've never seen a green fullscreen icon, so I'm confused what the exact
> steps to reproduce are. :-)

I think he's saying the blue fullscreen button, which appears at the right top corner if your mouse touch the top edge.

> > > > Dose Safari do that?
> > > Yes
> > 
> > Hmm, Safari does it far better. They hide the bookmarks and tabbar by
> > default and only display them when you hover the top. We probably want
> > something like that.
> 
> This is what we already do on Windows/Linux... there's a pref to turn it off
> (with a checkmark in the context menu), but otherwise, that's what we do.

They are different.

Safari dosen't completely hide all the controls. It keeps the address bar, but hides tab bar and bookmark bar. But if you hover the top edge to trigger the system menu bar, the tab bar and bookmark bar will be displayed.

What we did on Mac was, we always show all controls in fullscreen mode, but after bug 1162752, we hide part of the controls, and never display them in fullscreen mode like what Safari does. I suspect we want something like Safari, which shows more controls only when the system menu bar displays.
(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #4)
> > > > Dose Safari do that?
> > > Yes
> > 
> > Hmm, Safari does it far better. They hide the bookmarks and tabbar by
> > default and only display them when you hover the top. We probably want
> > something like that.
> 
> This is what we already do on Windows/Linux... there's a pref to turn it off
> (with a checkmark in the context menu), but otherwise, that's what we do.

That is not what I see Safari do on Yosemite and not what I think we should do in this case.

I wouldn't expect the UI to change significantly when going into fullscreen (as it is currently designed anyway) especially for something like the bookmarks bar which is already an opt-in toggle.

Safari *does* do a nicer job of exposing the menubar by sliding the entire UI down instead of just the titlebar. But that is outside the scope of this bug.
Flags: needinfo?(shorlander)
I think on that we should stick with the bahaviour we have in 38. (as shown in the uploaded video), in contrast to what we currently have in Nightly (previous video). As :shorlander mentions the bookmarks-bar is something the user chose to have displayed.
Flags: needinfo?(mjaritz)
Attached patch patch (obsolete) — Splinter Review
If you think bookmark toolbar should be displayed in fullscreen mode on Mac OS X, then that's fine. Easy fix.
Attachment #8616420 - Flags: review?(dao)
Comment on attachment 8616420 [details] [diff] [review]
patch

Seems like fullscreentoolbar="true" shouldn't be respected in that case in the first place, i.e. the fullscreen code should just skip the toolbar hiding part for Lion fullscreen mode.
Attachment #8616420 - Flags: review?(dao) → review-
Attached patch patchSplinter Review
Attachment #8616420 - Attachment is obsolete: true
Attachment #8616429 - Flags: review?(dao)
Attachment #8616429 - Flags: review?(dao) → review+
Comment on attachment 8616429 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1162752
[User impact if declined]: Some toolbars are not shown on Fullscreen mode on Mac
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: Not clear.
[String/UUID change made/needed]: n/a
Attachment #8616429 - Flags: approval-mozilla-aurora?
Assignee: nobody → quanxunzhen
Comment on attachment 8616429 [details] [diff] [review]
patch

Recent regression. Taking it as we would have time to find potential fallout from this change.
Attachment #8616429 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/9a2ffd3b6381
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Depends on: 1174431
Blocks: 1162752
No longer depends on: 1180492
Verified as fixed using Firefox 40 beta 2 and latest Aurora 41.0a2 2015-07-08 under Mac OS X 10.8.5.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.