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)

Product:
SeaMonkey
Component:
Location Bar
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(seamonkey2.10 fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.10
Tracking Flags:
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)

Addressed review comments
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
SeaMonkey developers are unlikely to work on this feature. However if someone supplies a suitable patch we won't reject it out of hand.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Just add setting for it (obsolete) — — Splinter Review 
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?
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-
2nd file
Attachment #602334 - Flags: review?(neil)
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+
Attached patch Tweaked patch (obsolete) — — Splinter Review 
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 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)
> +      <handler event="ValueChange"><![CDATA[
> * Actually hook up a caller (!)

I wonder why Dão put a Firefox urlbarBinding specific caller in toolkit autocomplete.
Depends on: 451833
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [parity=Firefox][parity-IE][parity-chrome][parity-opera]
(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]
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?
>> 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.
(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...

        Attachment #602373 -
        Attachment is obsolete: true

        Attachment #602643 -
        Flags: review?(philip.chee)

    
    

    
  
(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 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+

    
    

    
  
Pushed changeset 5cb1917f9106 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
    

    
  
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

    
    

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

    
  
Blocks: 732816
Depends on: 823499

    
    

    
  
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.

    
    

    
  
(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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: