Closed Bug 247603 Opened 20 years ago Closed 20 years ago

Removing address bar breaks stuff

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox1.0

People

(Reporter: jruderman, Assigned: jcginn)

References

Details

(Keywords: fixed-aviary1.0)

Attachments

(1 file, 5 obsolete files)

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)
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: