Closed Bug 247603 Opened 21 years ago Closed 20 years ago

Removing address bar breaks stuff

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox1.0

People

(Reporter: jruderman, Assigned: jcginn)

References

Details

(Keywords: fixed-aviary1.0)

Attachments

(1 file, 5 obsolete files)

unbitrotted patch
(deleted), patch
bugs
: review+
bugs
: approval-aviary+
Details | Diff | Splinter Review 
Steps to reproduce:
1. Remove the address bar using Customize Toolbars.
2. Switch tabs.

Result: The window title isn't updated.

Removing the address bar also makes the back/forward buttons always be disabled.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040614 Firefox/0.9

The backbutton works, but otherwise described as above
Bug 224182
Search bar doesn't work without Location bar.
Depends on: 224182
As I reported in bug 224182, this is an issue on Mac OS X as well. -> All/All

Also, the removal of the location bar causes all menus to become inactive (until
the browser is restarted) and temporarily removes all bookmarks from the menu
and the toolbar (this lingers until the location bar is restored and the browser
restarted again). So basically you need to quit the browser twice... once to get
menu access so you can restore the location bar, and a second time to restore
bookmarks.

This was tested on both OS X and Win XP using recently trunk nightlies. Perhaps
the branch behaves differently. Upgrading the severity, anyway, as it's pretty
horrible.

Not sure if it's a true dupe of bug 224182 or not, since these nastier effects
seemed to appear much later than the original search bar problem.
Severity: minor → critical
OS: Windows XP → All
Hardware: PC → All
Flags: blocking-aviary1.0?
If it's too difficult to fix this properly before 1.0, would it be possible to
put in a temporary patch that simply prevents the user removing the location
bar? The underlying problem could be addressed much later, then.
I think fixing this bug just requires adding "if (gURLBar)" in a few places.
Blocks: 259058
see Bug 259058 (both Aviary/trunk problem).
This really needs a fix soon (if it's that easy).
With the arrival of more newbees now 1.0PR is (about to be) released it would
generate too many bugreports.
also breaks:
-fullscreen mode
-open in new window from location dialog
Looking through lxr, there seem to be a lot of instances where gURLBar is
referenced without checking if it exists first. I wonder if it'd be better to
always have a urlbar, just keep it hidden if it's been removed from the
toolbars. Or is the current assumption that it exists a bad coding practice that
we should fix regardless?
*** Bug 261286 has been marked as a duplicate of this bug. ***
Target Milestone: --- → Firefox1.0
(In reply to comment #8)
> Looking through lxr, there seem to be a lot of instances where gURLBar is
> referenced without checking if it exists first. I wonder if it'd be better to
> always have a urlbar, just keep it hidden if it's been removed from the
> toolbars. Or is the current assumption that it exists a bad coding practice that
> we should fix regardless?

This also happens when javascript causes a pop-up to load without an address bar.

I was thinking it would be better if ctrl+t were to be disabled if the address
bar were'nt present (in the pop-up), in addition to all the keystrokes referring
to tabs, as well as context menu items on links, like [open link in new tab].

to try it out go to http://sputnik7.com and click on 'login'
when the login pops-up press ctrl+t, or ctrl+click on a link, then press
ctrl+tab, I would expect it to do nothing, but blank tabs are opened and the
address can't be changed so it's pointless to have it open.  I don't think it
really qualifies as 'critical', so you may want to move it to another bug
sorry it's so long.
This appears to be the potential places where the urlbar is refrenced without
checking if the urlbar is actually present. I'm not sure if this is every place
or if some of these will never be called if the bar is hidden, but I hope this
can help.

http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#1495
http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#1958
http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#2879
http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#3433
http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#3452
-> Several places in the onSecurityChange refers to this.urlBar
http://lxr.mozilla.org/aviarybranch/source/browser/base/content/search.xml#245
any evidence that PR users are hitting this?   it would be good to fix if the
patch appears and the patch is low risk.
Attached patch partial fix (obsolete) (deleted) — — Splinter Review 
This patch fixes all the breakage I am aware of relating to removing the
location bar EXCEPT for Bug 224182 (I know why this breaks, but the fix is a
bit more involved). I have tested this fairly extensively and haven't noticed
any problems.

I will admit that it is not the most robust solution, but it is better than the
current situation.
Comment on attachment 160559 [details] [diff] [review]
partial fix

Mike, I don't know if you want this, so please feel to pass this on to someone
else if you don't
Attachment #160559 - Flags: review?(mconnor)
Attached patch fix (obsolete) (deleted) — — Splinter Review 
I went ahead and also fixed the search bar. Not the most beautiful fix once
again, but it works.
Attachment #160559 - Attachment is obsolete: true
Attachment #160559 - Flags: review?(mconnor)
Attachment #160564 - Flags: review?(mconnor)
Attached patch fix (obsolete) (deleted) — — Splinter Review 
Actually - this is the real fix. Sorry for the bugspam.
Attachment #160564 - Attachment is obsolete: true
Attachment #160564 - Flags: review?(mconnor)
Attachment #160566 - Flags: review?(mconnor)
Thanks for taking this bug, Jim.

> +    event.preventDefault();

I think you meant aTriggeringEvent.  (Are preventDefault, etc. even necessary
there?)
Assignee: jruderman → vertigo451
You are correct. I will remove the event.* in the next patch. (For that matter
are they really needed in BrowserLoadURL? I get js console errors when I
cntrl-click on the location bar). I am still not very expereinced with the code
so mistakes like this may still be in the patch.
Attached patch better fix (obsolete) (deleted) — — Splinter Review 
This should fix any remaining bugs - including the problem that Jesse
mentioned. I also modified browser.xul so that the window control buttons are
always to the right side of the fullscreen toolbar (this is necessary for
consistency if no other element on the toolbar has the flex attribute, ie. when
the address bar is removed).
Attachment #160566 - Attachment is obsolete: true
Attachment #160566 - Flags: review?(mconnor)
Attachment #160593 - Flags: review?(mconnor)
Comment on attachment 160593 [details] [diff] [review]
better fix

ben can you review this if mconnor can ket to it soon?	we should make the
blocking call based on the review of the patch.
Attachment #160593 - Flags: superreview?(bugs)
There's no justification for shipping with this in 1.0.  I'm currently stuck
with a horribly broken window just because I moved the URL bar off the toolbar.

Patch looks good so far, just want to hammer on it a bit later, since I don't
have a build environment set up on win32 anymore.

+    if(!gURLBar)
+      return;
+  

spacing nit, if (foo) not if(foo), this is done in about six places or so,
please fix that before final checkin, no need for a new patch until Ben has his
look.
Flags: blocking-aviary1.0? → blocking-aviary1.0+
One edge case I missed.

- if (document.documentElement.getAttribute("chromehidden").indexOf("toolbar")
!= -1) {
+ if (document.documentElement.getAttribute("chromehidden").indexOf("toolbar")
!= -1 && gURLBar) {
    gURLBar.setAttribute("readonly", "true"); 
    gURLBar.setAttribute("enablehistory", "false");
  }

This looks like it only gets hit if you enable the navigation toolbar for pop-up
windows, thats why I missed it. I will whip up a new patch with this fix and the
nits Mike listed after someone looks over the code.
Comment on attachment 160593 [details] [diff] [review]
better fix

tested, looks good.  Note that its bitrotted already due to vlad's latest batch
of favicon fixes.
Attachment #160593 - Flags: review?(mconnor) → review+
I will attach an updated patch tonight. With your review should I continue to
ask sr Ben or just ask for approval aviary?
Attached patch updated patch (obsolete) (deleted) — — Splinter Review 
I fixed the spacing nits, added the one case I mentioned that I missed before,
and updated to Vlad's favicon changes.
Attachment #160593 - Attachment is obsolete: true
Attachment #160593 - Flags: superreview?(bugs)
Attachment #160889 - Flags: superreview?(bugs)
This has bitrotted yet again.
Whiteboard: [have patch] - need review ben
what's fullscreenflex added to the defaultsets?

and, what's the SearchLoadURL?
(In reply to comment #27)
> what's fullscreenflex added to the defaultsets?
When you remove the location bar from the navigation toolbar, you remove the
only default item with flex. As such, when you go to full screen, the now
visible window controls are not pushed all the way to the right, as expected,
but reside next to the rightmost toolbar item (this is the search bar by
default). Adding the fullscreenflex item (only shown in fullscreen) keeps the
window controls all the way to the right of the navigation toolbar and has a
negligible effect when the location bar (flex=1 vs. flex=100) is present.
Pictorial representations:

Fullscreen, un-customized, with/without flex added:
|[Navigation][<------location bar------>][search bar][full window controls]|

Fullscreen, no location bar, currently:
|[Navigation][search bar][full window controls]                            |
			    ^ notice the controls are no longer at the end of
                              the toolbar

Fullscreen, no location bar, with flex added:
|[Navigation][search bar][<-----fullscreenflex----->][full window controls]|

> and, what's the SearchLoadURL?
Currently, search urls from the search bar are launched by setting gURLBar.value
equal to the search url and calling BrowserLoadURL. This fails, obviously, if
the urlbar is not present. SearchLoadURL duplicates the functionality of
BrowserLoadURL, but allows the search bar to pass the url directly, without
setting gURLBar.value.
Attached patch unbitrotted patch (deleted) — — Splinter Review 
Since you are looking at the patch, Ben, I updated it. I also added back a
//XXX comment that I probably shouldn't remove (although I no longer believe it
to be valid anymore) and cleaned up the inputs of the SearchLoadURL.
Attachment #160889 - Attachment is obsolete: true
Attachment #160889 - Flags: superreview?(bugs)
Attachment #161324 - Flags: superreview?(bugs)
Comment on attachment 161324 [details] [diff] [review]
unbitrotted patch

r+a=ben@mozilla.org
Attachment #161324 - Flags: superreview?(bugs)
Attachment #161324 - Flags: review+
Attachment #161324 - Flags: approval-aviary+
*** Bug 224182 has been marked as a duplicate of this bug. ***
Since I do not have the proper permissions: Bug 252432, Bug 248002, and Bug
259058 should be fixed by this patch.
Blocks: 248002, 252432
Blocks: 224182
No longer depends on: 224182
Whiteboard: [have patch] - need review ben → [have patch] ready to land
Can someone check this in for me? I am worried about bitrot again. Sorry for
bugspam.
Ben, based on bonsai, it looks like you missed the changes to search.xml.
Keywords: fixed-aviary1.0
Ben checked in the search.xml file, too. 
*** Bug 248002 has been marked as a duplicate of this bug. ***
No longer blocks: 259058
*** Bug 259058 has been marked as a duplicate of this bug. ***
*** Bug 252432 has been marked as a duplicate of this bug. ***
No longer blocks: 252432
Whiteboard: [have patch] ready to land
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041010
Firefox/0.10

Believe this is part of bug, still seeing problem (in fullscreen).

Customize...
Create new toolbar and move addressbar to it.
Click done.
Customize...
Move addressbar to the right of the search bar.
Click done.
Press F11 for fullscreen.
Note the fullscreen min, max, close are in front of the addressbar, not alligned
to the right of the screen.

I don't think that's related to the url bar. I can reproduce it using other
icons instead, like Home, Stop, etc.

It looks like there's an invisible line in the main toolbar. If you place an
icon to the right of it, then it'll appear after the window controls in fullscreen.

In customize mode, when you drag icons to the left, you'll notice a large black
line/cursor appear. If you drop the icon at this point, then it'll appear to the
right of the window controls in fullscreen mode. If, however, you drag the icon
a little further to the left, so the line disappears, then release it, then
it'll appear to the left of the window control icons.

This definitely sounds like something that could be done better and should be
reported, but I don't think it's part of this bug.
Yes, the problem you have noticed is not the same as this bug and existed before
the fix for this bug. It has to deal with the fact that even if the window
controls (as well as fullscreenflex) are hidden, you can still customize the
toolbar and drag elements to the right of them. Feel free to file a bug about
this (check for dupes before of course).

The fix for this is to have the function showXULChrome make sure that the hidden
elements are always at the right end of the toolbar.
*** Bug 263798 has been marked as a duplicate of this bug. ***
As I reported in bug 263798, when removing the address bar and restarting 
firefox the home page does not come up, just a blank page.  I do not see any 
comments about this here, so I wanted to be sure it was noted.  Simply remove 
addressbar, restart firefox, and you will see a blank page.

Hi Ron,

It's also been reported as fixed in the latest nightlies. Have you tried one of
the latest?
I downloaded and installed firefoxsetup.exe from nightly 
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2004-10-10-07-trunk/

The same thing still happens, when you remove the address bar the home page 
does not load.  I apologize if this has been fixed but I do not understand how 
to patch it.  thanks.
You are correct, Rod. This fix has not landed on the trunk. Use a branch build
if you want to see this fixed.
My apologies. I misread it as fixed, when it was just set as a dupe.
This was fixed on the trunk when the aviary branch landed. I will watch for any
regressions, but when I tested the latest nightly everything looked good.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
QA Contact: bugzilla → toolbars
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: