Closed
Bug 247603
Opened 20 years ago
Closed 20 years ago
Removing address bar breaks stuff
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox1.0
People
(Reporter: jruderman, Assigned: jcginn)
References
Details
(Keywords: fixed-aviary1.0)
Attachments
(1 file, 5 obsolete files)
(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.
Comment 1•20 years ago
|
||
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.
Comment 3•20 years ago
|
||
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
Updated•20 years ago
|
Flags: blocking-aviary1.0?
Comment 4•20 years ago
|
||
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.
Reporter | ||
Comment 5•20 years ago
|
||
I think fixing this bug just requires adding "if (gURLBar)" in a few places.
Comment 6•20 years ago
|
||
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
Comment 8•20 years ago
|
||
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?
Comment 9•20 years ago
|
||
*** Bug 261286 has been marked as a duplicate of this bug. ***
Reporter | ||
Updated•20 years ago
|
Target Milestone: --- → Firefox1.0
Comment 10•20 years ago
|
||
(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.
Assignee | ||
Comment 11•20 years ago
|
||
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
Comment 12•20 years ago
|
||
any evidence that PR users are hitting this? it would be good to fix if the
patch appears and the patch is low risk.
Assignee | ||
Comment 13•20 years ago
|
||
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.
Assignee | ||
Comment 14•20 years ago
|
||
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)
Assignee | ||
Comment 15•20 years ago
|
||
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)
Assignee | ||
Comment 16•20 years ago
|
||
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)
Reporter | ||
Comment 17•20 years ago
|
||
Thanks for taking this bug, Jim.
> + event.preventDefault();
I think you meant aTriggeringEvent. (Are preventDefault, etc. even necessary
there?)
Assignee: jruderman → vertigo451
Assignee | ||
Comment 18•20 years ago
|
||
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.
Assignee | ||
Comment 19•20 years ago
|
||
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 20•20 years ago
|
||
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)
Comment 21•20 years ago
|
||
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+
Assignee | ||
Comment 22•20 years ago
|
||
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 23•20 years ago
|
||
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+
Assignee | ||
Comment 24•20 years ago
|
||
I will attach an updated patch tonight. With your review should I continue to
ask sr Ben or just ask for approval aviary?
Assignee | ||
Comment 25•20 years ago
|
||
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)
Assignee | ||
Comment 26•20 years ago
|
||
This has bitrotted yet again.
Updated•20 years ago
|
Whiteboard: [have patch] - need review ben
Comment 27•20 years ago
|
||
what's fullscreenflex added to the defaultsets?
and, what's the SearchLoadURL?
Assignee | ||
Comment 28•20 years ago
|
||
(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.
Assignee | ||
Comment 29•20 years ago
|
||
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 30•20 years ago
|
||
Attachment #161324 -
Flags: superreview?(bugs)
Attachment #161324 -
Flags: review+
Attachment #161324 -
Flags: approval-aviary+
Assignee | ||
Comment 31•20 years ago
|
||
*** Bug 224182 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 32•20 years ago
|
||
Since I do not have the proper permissions: Bug 252432, Bug 248002, and Bug
259058 should be fixed by this patch.
Updated•20 years ago
|
Updated•20 years ago
|
Whiteboard: [have patch] - need review ben → [have patch] ready to land
Assignee | ||
Comment 33•20 years ago
|
||
Can someone check this in for me? I am worried about bitrot again. Sorry for
bugspam.
Updated•20 years ago
|
Keywords: fixed-aviary1.0
Assignee | ||
Comment 34•20 years ago
|
||
Ben, based on bonsai, it looks like you missed the changes to search.xml.
Reporter | ||
Updated•20 years ago
|
Keywords: fixed-aviary1.0
Comment 35•20 years ago
|
||
Ben checked in the search.xml file, too.
Updated•20 years ago
|
Keywords: fixed-aviary1.0
Comment 36•20 years ago
|
||
*** Bug 248002 has been marked as a duplicate of this bug. ***
Comment 37•20 years ago
|
||
*** Bug 259058 has been marked as a duplicate of this bug. ***
Comment 38•20 years ago
|
||
*** Bug 252432 has been marked as a duplicate of this bug. ***
Comment 39•20 years ago
|
||
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.
Comment 40•20 years ago
|
||
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.
Assignee | ||
Comment 41•20 years ago
|
||
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.
Comment 42•20 years ago
|
||
*** Bug 263798 has been marked as a duplicate of this bug. ***
Comment 43•20 years ago
|
||
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.
Comment 44•20 years ago
|
||
Hi Ron,
It's also been reported as fixed in the latest nightlies. Have you tried one of
the latest?
Comment 45•20 years ago
|
||
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.
Assignee | ||
Comment 46•20 years ago
|
||
You are correct, Rod. This fix has not landed on the trunk. Use a branch build
if you want to see this fixed.
Comment 47•20 years ago
|
||
My apologies. I misread it as fixed, when it was just set as a dupe.
Assignee | ||
Comment 48•20 years ago
|
||
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
Updated•18 years ago
|
QA Contact: bugzilla → toolbars
You need to log in
before you can comment on or make changes to this bug.
Description
•