Last Comment Bug 689139 - Port |Bug 451833 - Highlight the domain name in the address bar| to Seamonkey
: Port |Bug 451833 - Highlight the domain name in the address bar| to Seamonkey
Status: RESOLVED FIXED
[parity-Firefox][parity-IE][parity-ch...
: ux-visual-hierarchy
Product: SeaMonkey
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: seamonkey2.10
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
: 184074 (view as bug list)
Depends on: 451833 823499
Blocks: 732816
  Show dependency treegraph
 
Reported: 2011-09-26 05:51 PDT by Phoenix
Modified: 2012-12-20 13:19 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Just add setting for it (1.18 KB, patch)
2012-03-02 04:54 PST, Phoenix
neil: review-
Details | Diff | Splinter Review
Changes in suite/browser/urlbarBindings.xml (4.47 KB, patch)
2012-03-02 06:13 PST, Phoenix
neil: review+
Details | Diff | Splinter Review
Tweaked patch (6.09 KB, patch)
2012-03-02 08:10 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Addressed review comments (6.27 KB, patch)
2012-03-03 12:14 PST, neil@parkwaycc.co.uk
philip.chee: review+
Details | Diff | Splinter Review

Description Phoenix 2011-09-26 05:51:02 PDT
What about porting mentioned bug to Seamonkey?
Comment 1 Philip Chee 2011-09-26 10:24:52 PDT
SeaMonkey developers are unlikely to work on this feature. However if someone supplies a suitable patch we won't reject it out of hand.
Comment 2 Philip Chee 2011-12-14 11:25:19 PST
*** Bug 184074 has been marked as a duplicate of this bug. ***
Comment 3 Phoenix 2012-03-02 04:54:25 PST
Created attachment 602313 [details] [diff] [review]
Just add setting for it

I checked sources and found, that almost all needed code already here in shared components, and only pref for highlighting missing, proposing trivial patch
Comment 4 neil@parkwaycc.co.uk 2012-03-02 05:12:40 PST
Comment on attachment 602313 [details] [diff] [review]
Just add setting for it

Sorry, but browser/base/content/urlbarBindings.xml isn't a shared component.
Comment 5 Phoenix 2012-03-02 06:13:01 PST
Created attachment 602334 [details] [diff] [review]
Changes in suite/browser/urlbarBindings.xml

2nd file
Comment 6 neil@parkwaycc.co.uk 2012-03-02 08:07:17 PST
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...
Comment 7 neil@parkwaycc.co.uk 2012-03-02 08:10:22 PST
Created attachment 602373 [details] [diff] [review]
Tweaked patch

Tweaks:
* Actually hook up a caller (!)
* Make the pref very live
* Reformat the URLbar after a blur
* Combine the two methods
Comment 8 Philip Chee 2012-03-03 02:10:36 PST
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?
Comment 9 Philip Chee 2012-03-03 02:22:21 PST
> +      <handler event="ValueChange"><![CDATA[
> * Actually hook up a caller (!)

I wonder why Dão put a Firefox urlbarBinding specific caller in toolkit autocomplete.
Comment 10 neil@parkwaycc.co.uk 2012-03-03 02:35:27 PST
(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...
Comment 11 Philip Chee 2012-03-03 02:40:37 PST
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 Philip Chee 2012-03-03 02:43:21 PST
>> 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.
Comment 13 neil@parkwaycc.co.uk 2012-03-03 06:54:59 PST
(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...
Comment 14 neil@parkwaycc.co.uk 2012-03-03 12:14:13 PST
Created attachment 602643 [details] [diff] [review]
Addressed review comments
Comment 15 Phoenix 2012-03-03 16:17:05 PST
(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 Philip Chee 2012-03-03 20:30:53 PST
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
Comment 17 neil@parkwaycc.co.uk 2012-03-04 07:35:23 PST
Pushed changeset 5cb1917f9106 to comm-central.
Comment 18 Jens Hatlak (:InvisibleSmiley) 2012-03-04 08:40:37 PST
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).
Comment 19 neil@parkwaycc.co.uk 2012-03-04 09:14:24 PST
(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 :-(
Comment 20 Frank Wein [:mcsmurf] 2012-12-20 12:57:04 PST
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 Jens Hatlak (:InvisibleSmiley) 2012-12-20 13:19:52 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.