1281493 - Identity popup sometimes doesn't display EV/DV site identity above "Secure connection" label

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[:Gijs (he/him)]

Seen on 48b1. I'm not sure what's triggering this yet - it works fine in a new window. It feels like it broke due to something or other, and now is permanently broken. Or something. :-\

Comment 1

[:Gijs (he/him)]

Attachment: Screenshot

Comment 2

[:Gijs (he/him)]

STR:

1. open Firefox
2. open the browser toolbox (don't open the identity popup before this)
3. search for the #identity-popup in the inspector
4. expand its kids/grandkids until you can see both the mainview and the security subview.
5. open the identity popup in the browser by clicking the identity block

I assume that the expansion of the tree under the element triggers XBL binding somehow. Obviously I didn't follow these exact steps when I first ran into this problem, but it's pretty plausible that I used the browser toolbox at some point in the uptime of the window where I first saw this, without ever having opened the identity popup (which I do almost never).

Per discussion on IRC, the problem seems to be that after the binding has been instantiated, the querySelectorAll call in the getter for the host/hostless labels finds only 1 of the 2 items, namely the one in the security subview, and ignores the one in the main view. I expect this has to do with the panel multiview moving the children / mainview around in the <stack> in its anonymous content.

Either way, I think we should avoid the code here behaving differently depending on when the panelmultiview binding for the popup gets instantiated.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

I wonder if bug 1207619 could have caused this.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Hey Jonathan any idea about this? Could it be caused by bug 1207619?

Comment 5

[Jonathan Kingston [:jkt] he/him]

Looks related yeah, will look into this.

Comment 6

[Jonathan Kingston [:jkt] he/him]

So I can't seem to find what is causing this however the reason it isn't showing now is related to the bug I worked on: bug 1207619

However this looks like a side effect of something else. For example running in toolbox console this selector should return two results: document.querySelectorAll(".identity-popup-headline.host")

Instead it only shows the more details section which hasn't been expanded. This seems to be similar to an inspecting issue I have had where using the inspector and highlighting the heading sometimes crashed out the inspector elements requiring me to manually expand them until it would step through and select it with the pointer.

Gijs has the heading not being shown without inspecting with developer mode at all?

Comment 7

[:Gijs (he/him)]

(In reply to Jonathan Kingston [:jkt] from comment #6)
> So I can't seem to find what is causing this however the reason it isn't
> showing now is related to the bug I worked on: bug 1207619
> 
> However this looks like a side effect of something else. For example running
> in toolbox console this selector should return two results:
> document.querySelectorAll(".identity-popup-headline.host")
> 
> Instead it only shows the more details section which hasn't been expanded.
> This seems to be similar to an inspecting issue I have had where using the
> inspector and highlighting the heading sometimes crashed out the inspector
> elements requiring me to manually expand them until it would step through
> and select it with the pointer.
> 
> Gijs has the heading not being shown without inspecting with developer mode
> at all?

I think it'll happen any time the binding gets instantiated at a different time than when showing the popup and/or this may be timing-dependent when the popup is shown (depends on the order of instantiation of bindings and/or when elements are shown/hidden). Either way, it feels fragile to me. As you noted, the qSA call always returns only 1 element instead of 2.

This likely has to do with how the panelUI binding moves <panelview> items around when it's instantiated, ie https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.xml#147-149 . I bet that manually checking both the main view and the subview container when querying for all applicable elements would fix this. We should do that for both the .host and the .hostless ones.

Comment 8

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1281493 - Moving element shuffle to when the popup is showing which prevents inspector XBL binding from breaking identity-popup. Removing surplus identity-popup-security-content selector.

Review commit: https://reviewboard.mozilla.org/r/60844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60844/

Comment 9

[Jonathan Kingston [:jkt] he/him]

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

Comment 10

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8765495 [details]
Bug 1281493 - Moving element shuffle to when the popup is showing which prevents inspector XBL binding from breaking identity-popup. Removing surplus identity-popup-security-content selector.

So this looks like it solves the issue, I triggered a try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcb36a1a829d

Let me know if you think this change will cause any issues.

Took a fair amount of learning how all the stack of elements relate to each other. Thanks for the pointers.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8765495 [details]
Bug 1281493 - Moving element shuffle to when the popup is showing which prevents inspector XBL binding from breaking identity-popup. Removing surplus identity-popup-security-content selector.

https://reviewboard.mozilla.org/r/60844/#review57832

I'm not convinced this is right, especially because try is also very unhappy, as far as I can tell.

::: browser/components/customizableui/content/panelUI.xml
(Diff revision 1)
> -        if (this._mainView) {
> -          this.setMainView(this._mainView);
> -        }

Moving this is pretty scary and also affects the main (hamburger) menu panel, especially because we're also moving it after syncContainerWithMainView, which seems wrong...

Why did you go for this approach rather than 'just' fixing the queryselector calls to explicitly look in the two subviews (main and security) ?

Comment 12

[Jonathan Kingston [:jkt] he/him]

No I'm not really convinced either, however some of the tests that broke initially worked on my computer others looked like timing issues which may relate to syncContainerWithMainView etc.

I tried going for that and it didn't seem to work, will try a few more selectors.

Comment 13

[:Gijs (he/him)]

(In reply to Jonathan Kingston [:jkt] from comment #12)
> No I'm not really convinced either, however some of the tests that broke
> initially worked on my computer others looked like timing issues which may
> relate to syncContainerWithMainView etc.
> 
> I tried going for that and it didn't seem to work, will try a few more
> selectors.

The selector is fine - it's the thing you're calling it on. You should be able to get a reference to the mainview container and its kid from the XBL binding of the panelmultiview, ditto for the subviews container.

Comment 14

[Jonathan Kingston [:jkt] he/him]

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

Comment 15

[Jonathan Kingston [:jkt] he/him]

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

Comment 16

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1281493 - Add another selector for identity popup main view as XBL bindings sometimes have a race condition. Removing surplus identity-popup-security-content selector.

Review commit: https://reviewboard.mozilla.org/r/61034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61034/

Comment 17

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8765890 [details]
Bug 1281493 - Add another selector for identity popup main view as XBL bindings sometimes have a race condition. Removing surplus identity-popup-security-content selector.

This is the one based on the selector method, thanks for the pointers. This works and neatens up the selectors etc.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8765890 [details]
Bug 1281493 - Add another selector for identity popup main view as XBL bindings sometimes have a race condition. Removing surplus identity-popup-security-content selector.

https://reviewboard.mozilla.org/r/61034/#review57844

Nice, thanks!

::: browser/base/content/browser.js:6597
(Diff revision 1)
> +    return document.getElementById("identity-popup-multiView");
> +  },
>    get _identityPopupContentHosts () {
>      delete this._identityPopupContentHosts;
> -    return this._identityPopupContentHosts = [...document.querySelectorAll(".identity-popup-headline.host")];
> +    let selector = ".identity-popup-headline.host";
> +    return this._identityPopupContentHosts = [...this._identityPopupMultiView._mainView.querySelectorAll(selector), ...document.querySelectorAll(selector)];

Nit: here and below, can you reformat as:

<stuff> = [
  ... <expr1>,
  ... <expr2>,
];

?

Comment 19

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8765890 [details]
Bug 1281493 - Add another selector for identity popup main view as XBL bindings sometimes have a race condition. Removing surplus identity-popup-security-content selector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61034/diff/1-2/

Comment 20

[Jonathan Kingston [:jkt] he/him]

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

Comment 21

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8765890 [details]
Bug 1281493 - Add another selector for identity popup main view as XBL bindings sometimes have a race condition. Removing surplus identity-popup-security-content selector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61034/diff/2-3/

Comment 22

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8765890 [details]
Bug 1281493 - Add another selector for identity popup main view as XBL bindings sometimes have a race condition. Removing surplus identity-popup-security-content selector.

A typo busted the tests, however this now passes those. Sorry about that. Can I get a review just to double check? Thanks!

Comment 23

[:Gijs (he/him)]

Comment on attachment 8765890 [details]
Bug 1281493 - Add another selector for identity popup main view as XBL bindings sometimes have a race condition. Removing surplus identity-popup-security-content selector.

https://reviewboard.mozilla.org/r/61034/#review57912

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/200d819719ee
Add another selector for identity popup main view as XBL bindings sometimes have a race condition. Removing surplus identity-popup-security-content selector. r=gijs

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/200d819719ee

Comment 26

[Paul Silaghi, QA [:pauly]]

Reproduced on FX 48b1.
Verified fixed FX 50.0a2 (2016-08-02) Win 7.