1324280 - Website certificate superimposed over navigation back/forward

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[David Ross]

Attachment: navigation.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20161217030205

Steps to reproduce:

Using Nightly on any website. After some activity the Tracking Protection/Website SSL Security Cert ends up superimposed over the top of the navigation buttons. Pretty consistent reproduction by simply going 'back' with my device native back key.


Actual results:

Clash of the icons


Expected results:

Everything should behave, just not today. Normal browser behaviour is what's expected.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

From the URL bar it looks like this is happening on a tablet? I do not see this on my N9. What device and Android version are you using?

Comment 2

[Carmen Fat [:cfat] - Ecosystem QA]

I can reproduce this issue using Asus ZenPad 8 (Android 6.0.1) and Asus Transformer Pad (Android 4.4.1).
On Nexus 9, I can't reproduce the issue.
Since I have no more tablet devices to test on, I can only assume that this is an Asus specific issue.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Is this a regression? What versions are affected?

Comment 4

[David Ross]

This is a Samsung Galaxy Tab A running Android 6.0.1 (Marshmallow). Behaviour only experienced in Nightly Fennec 53 so far (and occurring irregularly), but have downloaded 52, 51, and 50. Will test further. Odd behaviour with these icon has it spaced over to the right when not layered on top of each other. Uploading image 'RIGHT'

Comment 5

[David Ross]

Attachment: RIGHT - Icon now shifts to the right on occassion.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Max: Did the RTL patches touch the tablet toolbar and could have caused this?

Comment 7

[Max Liu [:maliu]]

(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> Max: Did the RTL patches touch the tablet toolbar and could have caused this?
It's possible caused by RTL patches. Though similar symptom only happen on RTL context, I'll take it.

Comment 8

[Max Liu [:maliu]]

Attachment: Bug 1324280 - Revert MarginLayoutParamsCompat with setting margin left/right by current layout direction,

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

Comment 9

[Bill Gianopoulos [:WG9s]]

I just changed this to major.  this looks really ugly.  I don't think a release with this behavior can go forward.

Comment 10

[Max Liu [:maliu]]

Comment on attachment 8820585 [details]
Bug 1324280 - Revert MarginLayoutParamsCompat with setting margin left/right by current layout direction,

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

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8820585 [details]
Bug 1324280 - Revert MarginLayoutParamsCompat with setting margin left/right by current layout direction,

https://reviewboard.mozilla.org/r/100078/#review100978

::: mobile/android/base/java/org/mozilla/gecko/toolbar/BrowserToolbarTablet.java:13
(Diff revision 2)
> +import org.mozilla.gecko.R;
> +import org.mozilla.gecko.animation.PropertyAnimator;
> +import org.mozilla.gecko.animation.ViewHelper;
> +

Seems like those lines moved around. Let's try to not do this to avoid conflicts with other patches/uplifts. :)

::: mobile/android/base/java/org/mozilla/gecko/toolbar/BrowserToolbarTablet.java:101
(Diff revision 2)
> -                    MarginLayoutParamsCompat.setMarginStart(layoutParams, 0);
> +                    if (isLayoutRtl()) {
> +                        layoutParams.rightMargin = 0;
> +                    } else {
> +                        layoutParams.leftMargin = 0;
> +                    }

I thought that's what MarginLayoutParamsCompat does internally for older versions? But here we want to always update the left/right margin?
A comment in the bug / commit message / code would be helpful to help me and others understand. :)

::: mobile/android/base/java/org/mozilla/gecko/toolbar/BrowserToolbarTablet.java:109
(Diff revision 2)
> -                    MarginLayoutParamsCompat.setMarginStart(layoutParams, 0);
> +                    if (isLayoutRtl()) {
> +                        layoutParams.rightMargin = 0;
> +                    } else {
> +                        layoutParams.leftMargin = 0;
> +                    }

This pattern is repeated multiple times - do we want a helper method like the one in the compat class?

Comment 12

[Max Liu [:maliu]]

Comment on attachment 8820585 [details]
Bug 1324280 - Revert MarginLayoutParamsCompat with setting margin left/right by current layout direction,

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

Comment 13

[Max Liu [:maliu]]

Comment on attachment 8820585 [details]
Bug 1324280 - Revert MarginLayoutParamsCompat with setting margin left/right by current layout direction,

https://reviewboard.mozilla.org/r/100078/#review101026

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d1a2b1de7f5
Revert MarginLayoutParamsCompat with setting margin left/right by current layout direction, r=sebastian

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d50229a6812
Revert MarginLayoutParamsCompat with setting margin left/right by current layout direction: lint style fixes. r=lint-fixes

Comment 16

[David Ross]

Using Nightly (2017-01-03) this is not resolved. Forgive my ignorance, is there delay before pushed to Nightly? (See new attachment navbug.png)

Comment 17

[David Ross]

Attachment: navbug.png

Bug persistent in Nightly (2017-01-03)

Comment 18

[Max Liu [:maliu]]

Hi David,

Patch landed days ago and I have verify latest nightly(2017-01-03) pass on emulator from platform 17 to 24. Seems symptom remains on 23(marshmallow) only.

May I confirm what device/platform are you using please? Thank you.

Comment 19

[David Ross]

Samsung 'Galaxy Tab A' SM-T555 running Android Marshmallow 6.0.1 (Build number MMB29M.T555XXU1BPDA)

Comment 20

[Max Liu [:maliu]]

Latest Nightly should include the solution patch. Please give it a try. Thank you.

Comment 21

[David Ross]

Wahoo! Thanks a bunch everyone. Excellent job. That was such a frustrating bug for me.