273926 - Can't change location in fullscreen mode when location bar is moved to different toolbar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jon Barnett]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

I have my location bar moved to the Menubar (File | Edit | etc..)
In fullscreen mode, the menubar gets hidden, as it should.
Since the location bar is hidden, I would expect Ctrl+L to open the Open
Location dialog.  It doesn't.  I assume that is highlights the contents of the
location bar, which is not hidden in full screen mode. 

Off-topic - I'd love to have the toolbars auto-hide in fullscreen mode...  other
posts have been made about this.

Reproducible: Always
Steps to Reproduce:
1.Click View > Toolbars > Customize...
2.Drag the Location bar from the Navigation Toolbar to the Menubar
3.Click "Done" in the Customize Window - return to browsing.
4.Press F11 or Click View > Full Screen
5.Press Ctrl+L

Actual Results:  
Nothing visible happens 
(Assumedly, the contents of the location bar are selected, though the menubar
and location bar are hidden)

Expected Results:  
Since the location bar is not visible, the Open Location dialog should appear

I don't have any kiosk-related extensions.  I tried "Almost Full Screen", but I
uninstalled it.

Comment 1

[Kevin Brosnan]

Most likely this can be duped to bug 171350.

Comment 2

[Jon Barnett]

Yes, I guess this would be almost a dupe of 171350, and should change accordingly.

In my opinion, I don't believe any new Toolbars are needed, and the toolbar used
in kiosk mode is fine.  I would just expect the Ctrl-L dialog to appear whent he
URL bar is hidden.  I'll post my opinion in the thread on 171350  

Jon B.

*** This bug has been marked as a duplicate of 171350 ***

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This patch adds an extra check in for the case where the toolbar that the urlbar is on is hidden due to fullscreen mode (get's moz-collapsed="true" added to it).

With this Ctrl-L will display the open dialog when the url bar is not visible in full screen mode.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Might as well give the search bar the same treatment while we're here.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 232573 [details] [diff] [review]
patch rev 2

Just check the visibility property of the computed style object.

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Right o. This reduces the number of tests needed.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 4

Visibility is inherited so it makes more sense to just check it on the bar itself and the computed style can be cached for both calls. Same applies to the search bar.

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch rev 5

Fix a minor mistake so we get the style for the search bar rather than the toolbar.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 232596 [details] [diff] [review]
patch rev 5

Make this a nested if in both places, i.e.

if (gFoo) {
  var style...
  ...
}

r=mano otherwise.

Comment 10

[Dave Townsend [:mossop]]

Attachment: patch rev 6

Done the nested if for the searchbar case but I can't see how to do this for the url bar since there needs to be a single if statement to test for the visibility of the bar so it can then fall through to the two following else cases.

So how about this, brings it together into one if by bringing the style assignment into the if test. The only other alternative I can think of is to not use else statements and instead use early returns if the bar is visible or the Mac case matches,

Comment 11

[Asaf Romano (gone)]

Comment on attachment 233110 [details] [diff] [review]
patch rev 6

earlier returns would be better (or, use the v5 way in openLocation).

Comment 12

[Dave Townsend [:mossop]]

Attachment: patch rev 7

This switches to using early returns in the openLocation case to allow for cleaner nested ifs.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 233355 [details] [diff] [review]
patch rev 7

r=mano

Comment 14

[Asaf Romano (gone)]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.677; previous revision: 1.676
done

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 233355 [details] [diff] [review]
patch rev 7

This is a fairly simple fix for an irritating issue. Would be nice if this could make the branch.

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 233355 [details] [diff] [review]
patch rev 7

a=mconnor on behalf of drivers for 1.8 branch checkin

Comment 17

[Dave Townsend [:mossop]]

Landed on 1.8 branch