1291730 - Move identity-block tooltip to connection icon and labels

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Johann Hofmann [:johannh]]

The fact that the identity-block has a "background tooltip" that shows "Verified by..." or "This website does not supply identity information." when you hover over the edges of icons with different tooltips is annoying and leads to constant confusion with QA. The text should only apply to the lock icon and the EV text next to it.

Comment 1

[Johann Hofmann [:johannh]]

We can make this a good first bugs since Dinarte was interested in taking this :)

Here's how to solve this: In http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#6970 we assign `tooltip` to the `identityBox` element. We don't want to do that anymore. Instead, we should assign `tooltip` to the elements with the ids "connection-icon" and "identity-icon-labels" (http://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#771)

Comment 2

[Dinarte Jesus [:jdj]]

Attachment: Assign background tooltip to connection-icon or identity-label instead of identityBox

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8777948 [details] [diff] [review]
Assign background tooltip to connection-icon or identity-label instead of identityBox

This looks good to me. I'll delegate to Florian for review.

One minor nit about the commit message: Technically it's "connection-icon AND identity-label" instead of "connection-icon OR identity-label".

Thanks!

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8777948 [details] [diff] [review]
Assign background tooltip to connection-icon or identity-label instead of identityBox

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

Thanks for taking care of this. The patch looks reasonable. I have one minor comment, as this bug is a good opportunity to further cleanup this tooltip :-).

::: browser/base/content/browser.js
@@ +6975,5 @@
>      }
>  
>      // Push the appropriate strings out to the UI
> +    this._connectionIcon.tooltipText = tooltip;
> +    this._identityIconLabels.tooltipText = tooltip;

Now that the tooltip is set on elements that are only visible for secure sites, there's no point in setting the tooltip value for the 'unknown' case, ie. this line can be removed:
  tooltip = gNavigatorBundle.getString("identity.unknown.tooltip");

And we can also remove the associated string from browser/locales/en-US/chrome/browser/browser.properties

Comment 5

[Dinarte Jesus [:jdj]]

Attachment: Assign background tooltip to connection-icon or identity-label instead and identityBox

Comment 6

[Dinarte Jesus [:jdj]]

Attachment: Assign background tooltip to connection-icon and identity-label instead of identityBox

Comment 7

[Dinarte Jesus [:jdj]]

Hello Florian for the spam, I made a mistake on the commit message. Thanks for the previews review.
Johann, I also fix the commit message like you ask.

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8778594 [details] [diff] [review]
Assign background tooltip to connection-icon and identity-label instead of identityBox

Looks good to me, thanks!

Comment 9

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=818d8c672196

Comment 10

[Johann Hofmann [:johannh]]

Great!

Dinarte, once a patch has r+ and all nits are fixed we usually push to the try server (https://wiki.mozilla.org/ReleaseEngineering/TryServer) to make sure all tests are still passing. I've taken care of that for you. We can also set the checkin-needed flag so this will get merged once everything looks green.

Comment 11

[Dinarte Jesus [:jdj]]

Cool, Johann!

Thanks to change the flag. Looking forward to the next bug. :D

Comment 12

[Johann Hofmann [:johannh]]

Mmh it looks like we need to investigate these failures on try before this can get checked in...

Comment 13

[Johann Hofmann [:johannh]]

So, it seems that this bug is not done just yet!

We caused a test to fail (incidentally one I wrote myself). That test verifies that the identity popup shows a correct certificate owner. What does that have to do with the tooltips? As it turns out, the "verifier" variable that sets this string in the identity popup takes its value from the tooltip on the identityBox. See [0][1]

How do we solve this? One might say we shouldn't rely on the tooltip to always contain the correct data in the first place, but that's a refactoring we should consider for a different bug.

I would suggest you simply check the links below and replace "this._identityBox" with one of the other two elements, e.g. "this._identityIconLabels".

[0] http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/base/content/browser.js#7129
[1] http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/base/content/browser.js#7136

To check the test that's failing simply run `./mach mochitest browser/base/content/test/general/browser_mixed_content_cert_override.js`.

Does that make sense, Dinarte? :)

Comment 14

[Dinarte Jesus [:jdj]]

Yes Johann, i'll fix this and create a new patch.

Comment 15

[Dinarte Jesus [:jdj]]

Hey Johann, I'm having an issue with running the test.

```
mozprofile.addons WARNING | Could not install /Users/dinartejesus/develop/contributions/mozilla-central/objdir-frontend/_tests/testing/mochitest/extensions/mozscreenshots: [Errno 2] No such file or directory: '/Users/dinartejesus/develop/contributions/mozilla-central/objdir-frontend/_tests/testing/mochitest/extensions/mozscreenshots/install.rdf'


!!! could not start server on port 8888: [Exception... "Component returned failure code: 0x804b0036 (NS_ERROR_SOCKET_ADDRESS_IN_USE) [nsIServerSocket.init]"  nsresult: "0x804b0036 (NS_ERROR_SOCKET_ADDRESS_IN_USE)"  location: "JS frame :: /Users/dinartejesus/develop/contributions/mozilla-central/objdir-frontend/dist/bin/components/httpd.js :: nsHttpServer.prototype._start :: line 550"  data: no]
```

Any ideas how to solve?

Comment 16

[Johann Hofmann [:johannh]]

Hi Dinarte,

Mochitest requires port 8888 open to run tests. You probably have another development server running on port 8888. Try closing all running dev servers or just restarting your terminal or your computer. :)

Comment 17

[Dinarte Jesus [:jdj]]

Attachment: Assign background tooltip to connection-icon and identity-label instead of identityBox

Comment 18

[Johann Hofmann [:johannh]]

Great, let's give this a hopefully final try!

Comment 19

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4964a431083

Comment 20

[Dinarte Jesus [:jdj]]

Yeah, Let's close this bug!

Comment 21

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/fx-team/rev/b4f5a80d3533c651b1a754a8f6bc71b0f5ad5ca5
Bug 1291730 - Assign background tooltip to connection-icon and identity-label instead of identityBox. r=florian r=johannh

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 23

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 51.0a1 (2016-08-23) Win 7, Ubuntu 14.04, OS X 10.12.