764405 - Misaligned door-hanger arrow with site-identity lock

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (06/13) - Screenshot

See screenshot.

Comment 1

[:Margaret Leibovic]

We hard-coded the position of that arrow, and it probably just needs to be updated to work with the new menu button.

Comment 2

[Sriram Ramasubramanian [:sriram]]

I've been thinking of moving DoorHanger to BrowserToolbar (as an interface), so that a layout / size changed in BrowserToolbar (like when tabs are shown), we can resize/reposition DoorHanger.

Comment 3

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #2)
> I've been thinking of moving DoorHanger to BrowserToolbar (as an interface),
> so that a layout / size changed in BrowserToolbar (like when tabs are
> shown), we can resize/reposition DoorHanger.

That definitely sounds like a better solution. SiteIdentityPopup uses its own layouts, so we'd also have to change that, or maybe we can make a shared arrow panel that we can position and fill up with different contents.

Comment 5

[Michael Monreal [:micmon]]

Don't know if it is worth a new bug so I just add it here for now: same problem also shows with the menu arrow pointing to the menu button, it's pointing to the left side of the icon instead of the center.

Comment 7

[:Margaret Leibovic]

(In reply to Michael Monreal [:monreal] from comment #5)
> Don't know if it is worth a new bug so I just add it here for now: same
> problem also shows with the menu arrow pointing to the menu button, it's
> pointing to the left side of the icon instead of the center.

Bug 768043 was filed about this.

Comment 8

[:Margaret Leibovic]

This was a regression from: http://hg.mozilla.org/mozilla-central/rev/1330924c44f2

Comment 9

[:Margaret Leibovic]

Attachment: patch

The margin moved from the ImageButton itself to its parent layout in lucasr's patch.

It feels fragile to be computing the margin in this way, but this fixes the immediate problem.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 639842 [details] [diff] [review]
patch

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

This looks good to me. We should find a better way to make all these robust.
I am thinking of some onSizeChangedListeners for the BrowserToolbar, which these PopupWindows can listen and update the icons.
This would be fine, the PopupWindow on the left will be affected with the shrinking/expanding of BrowserToolbar during showing/hiding TabsTray.

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6170836e4f9e

Comment 12

[:Margaret Leibovic]

Comment on attachment 639842 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression caused by bug 750707
User impact if declined: site identity popup is misaligned on phones
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk style change
String or UUID changes made by this patch: n/a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6170836e4f9e

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/adb33c04d59f

Comment 15

[:Margaret Leibovic]

Attachment: add back import on aurora

Code in Aurora depends on this import.

Comment 16

[:Margaret Leibovic]

Comment on attachment 642248 [details] [diff] [review]
add back import on aurora

I just pushed this to fix the bustage:
https://hg.mozilla.org/releases/mozilla-aurora/rev/67d61a9353d3