667586 - Clear identity block when typing a different URL into location bar

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Frank Yan (:fryn)]

Steps to reproduce:
1. Go to https://twitter.com
2. Focus the URL bar and start typing.

Expected results:
The identity box with the text "Twitter Inc." should disappear.

Actual results:
The identity box remains, which is confusing, because it looks like you're entering a search to Twitter or giving it a command.

Comment 1

[Frank Yan (:fryn)]

Alex, do you think it makes sense to do this now, or should we wait for the larger identity block changes that will come with the conditional forward button, etc.?

Comment 2

[Frank Yan (:fryn)]

(In reply to comment #1)

Never mind. After revising the bug to fix what is actually broken, it makes sense to do this now.

Comment 3

[Frank Yan (:fryn)]

Attachment: patch

On a non-verified page, typing in the URL bar makes the identity box revert to default favicon, no text, and gray background.
This simply makes it work the same way for verified pages to avoid mode errors.

Comment 4

[Dão Gottwald [:dao]]

This seems less than ideal when editing a URL rather than replacing it, which could be the reason why it wasn't done this way in the first place.

Comment 5

[Frank Yan (:fryn)]

(In reply to comment #4)
> This seems less than ideal when editing a URL rather than replacing it,
> which could be the reason why it wasn't done this way in the first place.

We're already clearing the favicon. If you're concerned about the shifting of the input text, how about setting visibility: hidden on #identity-icon-labels, instead of visibility: collapse?

The main thing is that we don't want the browser to look like it's in a domain-specific mode, because it's not.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> If you're concerned about the shifting of the input text,

Yes.

> how about setting visibility: hidden on
> #identity-icon-labels, instead of visibility: collapse?

Might look weird, but that's the only compromise I could think of as well.

Comment 7

[Dão Gottwald [:dao]]

Maybe the label could be really faint instead of hidden to prevent the identity box from looking broken.

Comment 8

[Frank Yan (:fryn)]

Attachment: method 1: clear text & icons but keep gray bg & border

Making that entire region blank felt weird, since it left the text floating in space, so I removed the text & icons while leaving the block's background and border.

Comment 9

[Frank Yan (:fryn)]

Attachment: method 2: hide the entire block, including its border

Here's the alternate approach of hiding the entire identity block (and the arrow panel icons to the left of it) including its border.

Feels weird, since it leaves the URL bar text floating a large number of pixels away from the left border of the URL bar.

Comment 10

[Frank Yan (:fryn)]

Attachment: method 1: clear text & icons but keep gray bg & border

s/visibility: hidden/opacity: 0/

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 542338 [details] [diff] [review]
method 1: clear text & icons but keep gray bg & border

This breaks the autocomplete activity icon. Also, clicking the identity block is supposed to restore the page's URL.

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

If it is easy enough to clear the identity block out of the ui, without shifting the text of the URL (just having it turn invible), then I'm fine with us doing this now.  It does bother me that it looks like you are sending information to the identity of the site that you are currently on.  If it isn't easy, we can just try to fix this with a bunch of proposed changes later on.

Comment 13

[Frank Yan (:fryn)]

(In reply to comment #12)
> If it is easy enough to clear the identity block out of the ui, without
> shifting the text of the URL (just having it turn invible), then I'm fine
> with us doing this now.

It's really easy. How clear should it be? Should the entire space taken up by the block  turn white, or should its contents simply become blank while retaining its background and border?

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

I don't have that much of a problem with the URL floating in the location bar with some odd white padding on the right side for now.  Not ideal, but I think it works better than a grey box that exists but contains no information.  We'll want to play with it on the UX branch though.

Comment 15

[Dão Gottwald [:dao]]

It's not clear to me that this bug even started with the right premise. While the identity box doesn't correspond to the location bar value anymore when editing it, which can hardly be a surprise to someone editing the value, it does correspond to the page. I see no reason to give this away when all proposed solutions are somehow weird in their own way.

Comment 16

[Frank Yan (:fryn)]

(In reply to Dão Gottwald [:dao] from comment #15)
> It's not clear to me that this bug even started with the right premise.
> While the identity box doesn't correspond to the location bar value anymore
> when editing it, which can hardly be a surprise to someone editing the
> value, it does correspond to the page.

We hide the bookmark star while the location bar value is being edited exactly because this UI is confusing.

If we end up moving the identity block as part with the changes for bug 674744, then this problem will also be fixed. I won't be working on this anymore until there's a clear direction following those changes.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug

This reverts the identity block to the generic state when the pageproxystate is invalid.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v2

This patch moves the attribute selector to the right-most selector so it will be a little more efficient, as well as removes the need to reset properties since it will now only apply the elevated identity states if the pageproxystate is valid.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 625313 [details] [diff] [review]
Patch for bug v2

As mentioned here before, shifting the input text via hiding #identity-icon-labels is undesirable. I have no good proposal what to do about this, other than not worrying about this bug; the icon always and reliably reflecting the current page's security status can easily be seen as a feature.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

I don't think the movement is as jarring as is claimed. This fixes an inconsistency within our UI and is consistent with Chrome and Opera.

Comment 23

[Frank Yan (:fryn)]

Comment on attachment 625313 [details] [diff] [review]
Patch for bug v2

I helped Jared produce this version of the patch and approve of it from a UX standpoint. This keeps the identity block consistent with the rest of the URL bar's state, especially when the URL bar is not focused.

Comment 24

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #22)
> I don't think the movement is as jarring as is claimed. This fixes an
> inconsistency within our UI and is consistent with Chrome and Opera.

It's not as consistent as you may think. The globe icon has a meaning as well, one that doesn't really make sense here. Chrome changes the icon to a search glass and blocks clicks on it, whereas we revert the location bar value and show the identity popup.

Comment 25

[Frank Yan (:fryn)]

(In reply to Dão Gottwald [:dao] from comment #24)
> Chrome changes the icon to a
> search glass and blocks clicks on it, whereas we revert the location bar
> value and show the identity popup.

It changes to a search glass to reflect that the current entered value will navigate to a search result page upon submission. We should do that (and blocking clicks) too, but our uriFixup code is currently non-deterministic (i.e. we don't know whether we will end up on a search results page b/c we try unqualified host lookup first), so we need to fix that first.

Comment 26

[Frank Yan (:fryn)]

To be clear, I don't mean at all that we should block this on fixing uriFixup.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v3

This version of the patch removes the ability to click on the identity block to revert the URL bar values.

If we land this patch, then I'll update bug 406779 to state that we have dropped this ability.

Comment 28

[Frank Yan (:fryn)]

Comment on attachment 627367 [details] [diff] [review]
Patch for bug v3

Review of attachment 627367 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +2650,5 @@
>    gProxyFavIcon.setAttribute("pageproxystate", aState);
>  
> +  var identityBox = document.getElementById("identity-box");
> +  if (identityBox)
> +    identityBox.setAttribute("pageproxystate", aState);

We already null-check for gURLBar, so remove this `if` condition.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

I kind of wish we could avoid spreading the use of the horribly-named "pageproxystate" attribute and use some other name, but I guess inconsistency with the URL bar attribute name would be a worse problem (and I can't really think of a better name).

Comment 30

[Frank Yan (:fryn)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #29)
> I kind of wish we could avoid spreading the use of the horribly-named
> "pageproxystate" attribute and use some other name, but I guess
> inconsistency with the URL bar attribute name would be a worse problem (and
> I can't really think of a better name).

I wish the boolean attribute didn't use "valid"/"invalid", making it not 100% clear that it's boolean, resulting in unnecessary `else if` conditions:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2657
And also makes it not obviously clear that the following states are the same:
`if (element.getAttribute("pageproxystate") != "valid")`
`element[pageproxystate="invalid"]`

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 627367 [details] [diff] [review]
Patch for bug v3

>+  var identityBox = document.getElementById("identity-box");
>+  if (identityBox)
>+    identityBox.setAttribute("pageproxystate", aState);

This isn't actually needed. You can just write #urlbar[pageproxystate=invalid] > #identity-box.

> #page-proxy-favicon[pageproxystate="invalid"] {
>-  opacity: 0.3;
>+  opacity: 0.5;
>+}

Why this change?

>+#identity-box[pageproxystate="invalid"] > #identity-box-inner > #identity-icon-labels {
>+  visibility: collapse;
>+}
>+
>+#identity-box[pageproxystate="invalid"] {
>+  pointer-events: none;
> }

Don't put this in theme code.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v4

(In reply to Dão Gottwald [:dao] from comment #31)
> Comment on attachment 627367 [details] [diff] [review]
> Patch for bug v3
>
> > #page-proxy-favicon[pageproxystate="invalid"] {
> >-  opacity: 0.3;
> >+  opacity: 0.5;
> >+}
> 
> Why this change?

I made this change for consistency between the three themes.
- Winstripe was set to 0.5 opacity when the blank favicon was updated to the dotted outline.
- Pinstripe was set to 0.5 opacity somewhere around Bug 587901 (which was backed out). The history is a little cloudy here because a lot of lines got moved around so I couldn't find a more definitive answer.
- Gnomestripe was set to 0.3 opacity by the patch for bug 621091, which switched Linux to using the stock icon for gtk files for blank documents instead of the default favicon.

Now that all platforms use the same icon for generic documents, I think we should standardize on the opacity of the icon for its disabled state.

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 627430 [details] [diff] [review]
Patch for bug v4

>     if ((event.type == "click" && event.button != 0) ||
>         (event.type == "keypress" && event.charCode != KeyEvent.DOM_VK_SPACE &&
>-         event.keyCode != KeyEvent.DOM_VK_RETURN))
>+         event.keyCode != KeyEvent.DOM_VK_RETURN) ||
>+        (gURLBar.getAttribute("pageproxystate") != "valid"))
>       return; // Left click, space or enter only

The comment doesn't match the code anymore. Maybe you should mention why the pageproxystate check is needed even though you already set pointer-events: none.
You could also make this a separate if (...) return; block or add it to if (this._mode == this.IDENTITY_MODE_CHROMEUI) for the sake of the above block's readability.

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

> #page-proxy-favicon[pageproxystate="invalid"] {
>-  opacity: 0.3;
>+  opacity: 0.5;
> }

Since the globe icon doesn't represent an identity state anymore and since the icon isn't clickable anymore, making winstripe and pinstripe follow gnomestripe might make more sense.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v5

Addressed feedback. Yeah, I think we can use the more transparent 0.3 value.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 627459 [details] [diff] [review]
Patch for bug v5

>+    // Don't allow enter or space if the location is
>+    // chrome UI or the location has been modified.
>+    if (this._mode == this.IDENTITY_MODE_CHROMEUI ||
>+        gURLBar.getAttribute("pageproxystate") != "valid")
>       return;

In the case of IDENTITY_MODE_CHROMEUI, this covers left clicks as well as enter or space.

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af889781505c

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/af889781505c