Closed
Bug 689139
Opened 13 years ago
Closed 12 years ago
Port |Bug 451833 - Highlight the domain name in the address bar| to Seamonkey
Categories
(SeaMonkey :: Location Bar, enhancement)
SeaMonkey
Location Bar
Tracking
(seamonkey2.10 fixed)
RESOLVED
FIXED
seamonkey2.10
Tracking | Status | |
---|---|---|
seamonkey2.10 | --- | fixed |
People
(Reporter: mz+bugzilla, Assigned: neil)
References
Details
(Keywords: ux-visual-hierarchy, Whiteboard: [parity-Firefox][parity-IE][parity-chrome][parity-opera])
Attachments
(1 file, 3 obsolete files)
6.27 KB,
patch
|
philip.chee
:
review+
|
Details | Diff | Splinter Review |
What about porting mentioned bug to Seamonkey?
Severity: normal → enhancement
Keywords: ux-visual-hierarchy
Comment 1•13 years ago
|
||
SeaMonkey developers are unlikely to work on this feature. However if someone supplies a suitable patch we won't reject it out of hand.
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
I checked sources and found, that almost all needed code already here in shared components, and only pref for highlighting missing, proposing trivial patch
Attachment #602313 -
Flags: review?
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 602313 [details] [diff] [review] Just add setting for it Sorry, but browser/base/content/urlbarBindings.xml isn't a shared component.
Attachment #602313 -
Flags: review? → review-
Assignee | ||
Comment 6•12 years ago
|
||
Comment on attachment 602334 [details] [diff] [review] Changes in suite/browser/urlbarBindings.xml It's mostly OK but it's missing a few bits. Tweaked patch coming up...
Attachment #602334 -
Flags: review?(neil) → review+
Assignee | ||
Comment 7•12 years ago
|
||
Tweaks: * Actually hook up a caller (!) * Make the pref very live * Reformat the URLbar after a blur * Combine the two methods
Assignee: nobody → neil
Attachment #602313 -
Attachment is obsolete: true
Attachment #602334 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #602373 -
Flags: review?(philip.chee)
Comment 8•12 years ago
|
||
Comment on attachment 602373 [details] [diff] [review] Tweaked patch This patch works. One minor UI nit. If you have dynamic skin switching on and switch themes using View->Apply Theme, the highlighting disappears. Some coding nits: > + } else if (aPref == "browser.urlbar.formatting.enabled") { > + this._formattingEnabled = this.mPrefs.getBoolPref("browser.urlbar.formatting.enabled"); How about? this._formattingEnabled = this.mPrefs.getBoolPref((aPref); > + <method name="_formatValue"> > + <parameter name="formattingEnabled"/> > + <body><![CDATA[ > + let controller = this.editor.selectionController; I know this is just copy and paste from Firefox, but I note that this file doesn't use let's in general. So please replace all these let's with var's wherever applicable. > + ["http:", "https:", "ftp:"].indexOf(protocol[0]) == -1) !/^(https?|ftp):/.test(protocol[0]); > + let segments = function (s) s.replace(/[^.]*/g, "").length + 1; function (s) { .... }; Do we need a non-anonymous function here? function segments(s) {...}; > + <handler event="blur" phase="capturing"><![CDATA[ Is capturing needed here?
Attachment #602373 -
Flags: review?(philip.chee)
Comment 9•12 years ago
|
||
> + <handler event="ValueChange"><![CDATA[
> * Actually hook up a caller (!)
I wonder why Dão put a Firefox urlbarBinding specific caller in toolkit autocomplete.
Updated•12 years ago
|
Depends on: 451833
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [parity=Firefox][parity-IE][parity-chrome][parity-opera]
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Philip Chee from comment #8) > This patch works. One minor UI nit. If you have dynamic skin switching on > and switch themes using View->Apply Theme, the highlighting disappears. OK, I didn't think of that. Let me see what I can do. > Some coding nits: > > + } else if (aPref == "browser.urlbar.formatting.enabled") { > > + this._formattingEnabled = this.mPrefs.getBoolPref("browser.urlbar.formatting.enabled"); > How about? > this._formattingEnabled = this.mPrefs.getBoolPref((aPref); Well, I guess I should change the other prefs in that case... > > + <method name="_formatValue"> > > + <parameter name="formattingEnabled"/> > > + <body><![CDATA[ > > + let controller = this.editor.selectionController; > I know this is just copy and paste from Firefox, but I note that this file > doesn't use let's in general. So please replace all these let's with var's > wherever applicable. I did wonder about that, but then I thought that was getting too far away from the original patch... > > + ["http:", "https:", "ftp:"].indexOf(protocol[0]) == -1) > !/^(https?|ftp):/.test(protocol[0]); Fair enough. > > + let segments = function (s) s.replace(/[^.]*/g, "").length + 1; > function (s) { .... }; > Do we need a non-anonymous function here? > function segments(s) {...}; Good catch! > > + <handler event="blur" phase="capturing"><![CDATA[ > Is capturing needed here? Yes, the blur event doesn't bubble. (The focusout event would, if we had it.) (In reply to Philip Chee from comment #9) > > > + <handler event="ValueChange"><![CDATA[ > > * Actually hook up a caller (!) > I wonder why Dão put a Firefox urlbarBinding specific caller in toolkit > autocomplete. So did I...
Whiteboard: [parity=Firefox][parity-IE][parity-chrome][parity-opera] → [parity-Firefox][parity-IE][parity-chrome][parity-opera]
Comment 11•12 years ago
|
||
From Bug 184074 Comment 26: > Looks like IE8 is implementing this > http://blogs.msdn.com/ie/archive/2008/03/11/address-bar-improvements-in-internet-explorer-8-beta-1.aspx "Domain Highlighting will 'disappear' if the user hovers over or clicks in the Address Bar." Neither we nor Firefox listen to hover. What does Chrome do?
Comment 12•12 years ago
|
||
>> Is capturing needed here?
> Yes, the blur event doesn't bubble. (The focusout event would, if we had it.)
The reason I asked is that the Firefox version doesn't use capturing.
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Philip Chee from comment #8) > (From update of attachment 602373 [details] [diff] [review]) > > + <handler event="blur" phase="capturing"><![CDATA[ > Is capturing needed here? In fact it isn't needed; smaug changed the way event handling worked, but incidentally caused an unrelated regression. Oops...
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #602373 -
Attachment is obsolete: true
Attachment #602643 -
Flags: review?(philip.chee)
Reporter | ||
Comment 15•12 years ago
|
||
(In reply to Philip Chee from comment #11) > Neither we nor Firefox listen to hover. What does Chrome do? Firefox highlights address bar on hover, Chrome 17 do nothing
Comment 16•12 years ago
|
||
Comment on attachment 602643 [details] [diff] [review] Addressed review comments Cryptic comments strike again. When I said "replace all these let's with var's wherever applicable" I meant to imply that let's within sub-scopes shouldn't be changed. I'm not too torqued about this however. r=me
Attachment #602643 -
Flags: review?(philip.chee) → review+
Assignee | ||
Comment 17•12 years ago
|
||
Pushed changeset 5cb1917f9106 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 18•12 years ago
|
||
I think this needs a matching option in Preferences. Please file a follow-up bug for that, depending on this one. In the meantime I set the relnote keyword so we can take care of at least explaining it (and how to disable it; I'm almost sure some of our more conservative users will ask).
Keywords: relnote
Target Milestone: --- → seamonkey2.10
Assignee | ||
Comment 19•12 years ago
|
||
(In reply to Jens Hatlak from comment #18) > I think this needs a matching option in Preferences. Please file a follow-up > bug for that, depending on this one. Yeah, I meant to do that already, but I forgot :-(
Updated•12 years ago
|
status-seamonkey2.10:
--- → fixed
Comment 20•12 years ago
|
||
This bug here added some code which throws a JS error (this.editor is null) when opening a popup via window.open (maybe related to the popup not having an address bar?). See Bug 823499 for a testcase.
Comment 21•12 years ago
|
||
(In reply to Frank Wein [:mcsmurf] from comment #20) > This bug here added some code which throws a JS error (this.editor is null) > when opening a popup via window.open (maybe related to the popup not having > an address bar?). If that was the reason, maybe FF is not affected since it forces the address bar to be visible (but not editable) for popups.
You need to log in
before you can comment on or make changes to this bug.
Description
•