Closed Bug 947972 Opened 6 years ago Closed 6 years ago

Add checkbox to toggle https: background color in Location Bar preference pane

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.27

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

Details

Attachments

(2 files, 3 obsolete files)

(Quoting rsx11m from bug 354940 comment #13)
> (In reply to rsx11m from bug 354940 comment #9)
> > Some boolean pref might be easier and could be evaluated in
> > nsBrowserStatusHandler.js when setting the security level in the
> > onSecurityChange() function, but would imply a bit of overhead and yet
> > another browser.urlbar.* pref.
> 
> That was actually fairly straightforward to implement. This patch adds a
> hidden preference browser.urlbar.highlight.secure to toggle the special
> background color for secure connection. This is achieved by simply not
> setting the "level" attribute if that preference is false.
> 
> If we want to go with that preference, it would be easy to add a checkbox to
> the Location Bar prefpane (probably in a follow-up bug, different component).

Since I've worked out that patch while waiting for the review for the main patch already, here it comes. It places the browser.urlbar.highlight.secure right underneath browser.urlbar.formatting.enabled into the same groups (which at this point has a single box only anyway and it fits well there context-wise).

There was a bit of doubt at the beginning of the bug 354940 on this preference, but eventually the consensus was that it's a good thing to have. I don't see a reason why the checkbox for formatting the URL bar should be provided while one for displaying secure sites with a different background color shouldn't, they seem to be both on the same contextual level.
Attached patch Proposed patch (obsolete) — Splinter Review
Adds checkbox to Location Bar prefs and updates Help content.
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #8344692 - Flags: ui-review?(neil)
Attachment #8344692 - Flags: review?(iann_bugzilla)
Attached image Screenshot of pane (obsolete) —
On Linux using KDE4's default desktop theme for a change.
Comment on attachment 8344692 [details] [diff] [review]
Proposed patch

This makes that pane taller of course... maybe we can move the Internet Search checkbox into the Autocomplete group?

>+      <li><strong>Highlight web pages with a high level of connection
>+        security</strong>: Shows a dedicated background color the next time a
>+        web page is loaded if all of its components are fully encrypted.</li>
I would have just said something along the lines of "Identify fully encrypted pages by highlighting the location bar in yellow."
(In reply to neil@parkwaycc.co.uk from comment #3)
> This makes that pane taller of course... maybe we can move the Internet
> Search checkbox into the Autocomplete group?

It fits on Windows Classic with GDI fonts, but may stretch it with the larger DirectWrite fonts. The Internet Search menuitem shows up with an Autocomplete, thus I'd agree that moving "Show default search engine" to the bottom of that group (after "Show list of matching results") makes sense.

As for the Help text, "if all of its components are fully encrypted" was intended to make sure that the user understands it won't apply in mixed-content situations; "next time a web page is loaded" was chosen to avoid the impression that changing the checkbox would have immediate effect (there was a patch for that in the other bug which got an f- given that a pref listener was considered an overkill for just that case).
Hmm, do we really want to expose this? More UI clutter and messing with security relevant prefs can also be done via about:config (I'm not a fan of adding every possible pref to the pref window...).
Well, maybe. But why did we expose browser.urlbar.formatting.enabled as a checkbox then? As said, I think that those are on the same level, and exposing one but not the other is rather confusing than having it consistent at least.
We could also remove the browser.urlbar.formatting.enabled checkbox :)
Sure, but you could say the same about a lot of other prefs. Firefox and Thunderbird have tended to a rather minimalistic UI in the past whereas SeaMonkey also offers options in the UI which aren't necessarily used by "everybody" - as for security relevance, the highlighting is a theme property (so you may say is the visual emphasis on the domain part in the other option), and some will like it while others (as we know) won't...
(In reply to rsx11m from comment #6)
> But why did we expose browser.urlbar.formatting.enabled as a
> checkbox then?

I'd say because people (read: our users) are more likely to dislike the domain highlighting (which effectively tints the rest of the URL). SM didn't show this until it was introduced, which was a long time. Secure website background coloring OTOH is rather undisputed, i.e. I cannot remember to have heard or read about anyone complaining about it. And as long as we keep an eye on contrast between background and foreground colors, I don't expect that to change.

IOW to me it comes down to the question how likely it is that a reasonable portion of our users would like to revert the decision we took for them. If it is just a tiny fraction and the issue is not too sensitive, I think a hidden pref will do.

Just my 2c.
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #9)
> Secure website background coloring OTOH is rather undisputed, i.e. I cannot
> remember to have heard or read about anyone complaining about it.

That's indeed why a couple of users in bug 354940 commented. Also keep in mind that the background color is now a tick more intense than before, thus users who didn't bother before may now be inclined to remove the effect (e.g., due to perceived or actual loss of contrast, as some argued).

> IOW to me it comes down to the question how likely it is that a reasonable
> portion of our users would like to revert the decision we took for them.

That's a tricky assessment without hard data from whatever feedback we may or may not get (usually bugs filed or threads posted in the MZ forums). Anyway, the main reason for me to post this patch now was because I had it on my shelf and the topic itself is still fresh in my mind. If we want to delay a decision until some feedback comes in, that's fine with me, but would defer this by three release cycles at least.

As for the "clutter" argument: Neil's suggestion in comment #3 to move the single-box Internet Search group into the Autocomplete group would rather unclutter this pane, so I'd definitely go with that one.
Attached patch Proposed patch (v2) (obsolete) — Splinter Review
Updated patch with comment #3 addressed, except that I've kept the phrases in the Help content for full encryption and the delay in being effective with the next page only.
Attachment #8344692 - Attachment is obsolete: true
Attachment #8344693 - Attachment is obsolete: true
Attachment #8344692 - Flags: ui-review?(neil)
Attachment #8344692 - Flags: review?(iann_bugzilla)
Attachment #8344900 - Flags: ui-review?(neil)
Attachment #8344900 - Flags: review?(iann_bugzilla)
Attached image Screenshot (v2)
Updated per attachment 8344900 [details] [diff] [review]. Note that Windows has a different indentation of the checkboxes in the Autocomplete group, that's on purpose and not an effect of this patch.
Comment on attachment 8344900 [details] [diff] [review]
Proposed patch (v2)

>--- a/suite/locales/en-US/chrome/common/pref/pref-locationbar.dtd
>+++ b/suite/locales/en-US/chrome/common/pref/pref-locationbar.dtd
>+<!ENTITY showSearchEngine.label                "Show default search engine">
>+<!ENTITY showSearchEngine.accesskey            "e">

>-<!ENTITY searchEngine.label                    "Internet Search Engine">
>-<!ENTITY showSearchEngine.label                "Show default search engine">
>-<!ENTITY showSearchEngine.accesskey            "e">

Hmm, I'm wondering - if we stick with this move, the context of the showSearchEngine.label in "Internet Search Engine" is lost in the Autocomplete group. Thus, should that label be rather "Show default Internet search engine" instead to be unambiguous what is searched where in the autocomplete list?
Attachment #8344900 - Flags: review?(iann_bugzilla) → review+
Neil, any opinion on this?

I see where Frank an Jens are coming from, but I also see where I'm coming from ;-) (and IanN seems to agree given that he approved the patch last week). I'd still see it on the "nice to have" list as argued in my replies, and getting rid of the two single-checkbox groups (which look a bit odd at least on Windows Classic with the group borders) should be beneficial for this pane as well.
> Thus, should that label be rather "Show default Internet search engine"
> instead to be unambiguous what is searched where in the autocomplete list?

This extends attachment 8344900 [details] [diff] [review] with this label change for easier review.
Attachment #8365721 - Flags: ui-review?(neil)
Attachment #8365721 - Flags: review+
Comment on attachment 8365721 [details] [diff] [review]
Alternative patch per comment #13

Sorry for the atrocious delay. I prefer this version, thanks.
Attachment #8365721 - Flags: ui-review?(neil) → ui-review+
Comment on attachment 8344900 [details] [diff] [review]
Proposed patch (v2)

I assume this one isn't interesting any more.
Attachment #8344900 - Flags: ui-review?(neil)
Attachment #8344900 - Attachment is obsolete: true
Thanks Neil, push of attachment 8365721 [details] [diff] [review] for comm-central please.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/1b35ab70f951
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.27
Blocks: 1018792
No longer blocks: 1018792
You need to log in before you can comment on or make changes to this bug.