Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Clear identity block when typing a different URL into location bar

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Location Bar
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: fryn, Assigned: jaws)

Tracking

(Depends on: 5 bugs, {ux-mode-error})

Trunk
Firefox 15
ux-mode-error
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-opera][parity-chrome])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Summary: Identity box and icons should be collapsed when user is inputting a different URL into the location bar → Collapse identity box & icons when typing a different URL into location bar
(Reporter)

Comment 1

6 years ago
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.?
Summary: Collapse identity box & icons when typing a different URL into location bar → Collapse text in identity block when typing a different URL into location bar
(Reporter)

Comment 2

6 years ago
(In reply to comment #1)

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

Comment 3

6 years ago
Created attachment 542247 [details] [diff] [review]
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.
Attachment #542247 - Flags: review?(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.
(Reporter)

Comment 5

6 years ago
(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.
(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.
Maybe the label could be really faint instead of hidden to prevent the identity box from looking broken.
(Reporter)

Updated

6 years ago
Summary: Collapse text in identity block when typing a different URL into location bar → Clear text and icons in identity block when typing a different URL into location bar
(Reporter)

Comment 8

6 years ago
Created attachment 542324 [details] [diff] [review]
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.
Attachment #542247 - Attachment is obsolete: true
Attachment #542324 - Flags: ui-review?(faaborg)
Attachment #542324 - Flags: review?(dao)
Attachment #542247 - Flags: review?(dao)
(Reporter)

Updated

6 years ago
Summary: Clear text and icons in identity block when typing a different URL into location bar → Clear identity block when typing a different URL into location bar
(Reporter)

Comment 9

6 years ago
Created attachment 542325 [details] [diff] [review]
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.
(Reporter)

Comment 10

6 years ago
Created attachment 542338 [details] [diff] [review]
method 1: clear text & icons but keep gray bg & border

s/visibility: hidden/opacity: 0/
Attachment #542324 - Attachment is obsolete: true
Attachment #542338 - Flags: ui-review?(faaborg)
Attachment #542338 - Flags: review?(dao)
Attachment #542324 - Flags: ui-review?(faaborg)
Attachment #542324 - Flags: review?(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.
Attachment #542338 - Flags: review?(dao) → review-
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.
(Reporter)

Comment 13

6 years ago
(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?
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.
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.
(Reporter)

Updated

6 years ago
Attachment #542338 - Flags: ui-review?(faaborg)
(Reporter)

Comment 16

6 years ago
(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.
Assignee: fryn → nobody
Status: ASSIGNED → NEW

Updated

6 years ago
Duplicate of this bug: 711761

Updated

5 years ago
Duplicate of this bug: 754498
(Reporter)

Updated

5 years ago
Attachment #542338 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #542325 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Depends on: 742419
Created attachment 625287 [details] [diff] [review]
Patch for bug

This reverts the identity block to the generic state when the pageproxystate is invalid.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #625287 - Flags: review?(fryn)
Created attachment 625313 [details] [diff] [review]
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.
Attachment #625287 - Attachment is obsolete: true
Attachment #625313 - Flags: review?(fryn)
Attachment #625287 - Flags: review?(fryn)
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.
Attachment #625313 - Flags: review?(fryn) → review-
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.
Whiteboard: [parity-opera][parity-chrome]
Attachment #625313 - Flags: review- → review?(gavin.sharp)
(Reporter)

Comment 23

5 years ago
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.
Attachment #625313 - Flags: ui-review?(shorlander)
Attachment #625313 - Flags: feedback+
(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.
(Reporter)

Comment 25

5 years ago
(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.
(Reporter)

Comment 26

5 years ago
To be clear, I don't mean at all that we should block this on fixing uriFixup.
Attachment #625313 - Flags: ui-review?(shorlander)
Attachment #625313 - Flags: review?(gavin.sharp)
Created attachment 627367 [details] [diff] [review]
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.
Attachment #625313 - Attachment is obsolete: true
Attachment #627367 - Flags: review?(fryn)
Attachment #627367 - Flags: feedback?(shorlander)
(Reporter)

Comment 28

5 years ago
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.
Attachment #627367 - Flags: ui-review?(shorlander)
Attachment #627367 - Flags: review?(fryn)
Attachment #627367 - Flags: review+
Attachment #627367 - Flags: feedback?(shorlander)
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).
(Reporter)

Comment 30

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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"]`
Whiteboard: [parity-opera][parity-chrome] → [parity-opera][parity-chrome][autoland-try:-b d -p linux -u mochitests -t none]

Updated

5 years ago
Whiteboard: [parity-opera][parity-chrome][autoland-try:-b d -p linux -u mochitests -t none] → [parity-opera][parity-chrome][autoland-in-queue]
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.
Attachment #627367 - Flags: review-
Created attachment 627430 [details] [diff] [review]
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.
Attachment #627367 - Attachment is obsolete: true
Attachment #627367 - Flags: ui-review?(shorlander)
Attachment #627430 - Flags: review?(dao)
Attachment #627367 - Flags: review+
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.
Created attachment 627459 [details] [diff] [review]
Patch for bug v5

Addressed feedback. Yeah, I think we can use the more transparent 0.3 value.
Attachment #627430 - Attachment is obsolete: true
Attachment #627430 - Flags: review?(dao)
Attachment #627459 - Flags: review?(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.
Attachment #627459 - Flags: review?(dao) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/af889781505c
Flags: in-testsuite-
Whiteboard: [parity-opera][parity-chrome][autoland-in-queue] → [parity-opera][parity-chrome]
Target Milestone: --- → Firefox 15

Updated

5 years ago
Depends on: 759060
https://hg.mozilla.org/mozilla-central/rev/af889781505c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 397594

Updated

5 years ago
Depends on: 777038
Duplicate of this bug: 267146

Updated

4 years ago
Depends on: 891833

Updated

4 years ago
Depends on: 911322
Depends on: 908110

Updated

2 years ago
Depends on: 1231025
You need to log in before you can comment on or make changes to this bug.