1350661 - Firefox menu button covers the entire URL bar on RTL builds of Firefox beta on a 4.2.2 android device

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Itiel]

Device: Samsung Galaxy S3 Mini GT-I8200N
Android version: 4.2.2
Latest Firefox Beta installed from Google Play

Attached screenshot is the result.
The menu button covers (almost) the entire URL bar, making Firefox practically unusable.

Not sure whether this should be fixed as we knew from the start that supporting RTL will have its setbacks on older android versions but this seems to me like a critical bug.

Changing the UI language to english fixes the issue.

Comment 1

[Tomer Cohen :tomer]

(In reply to ItielMaN from comment #0)
> Attached screenshot is the result.
I afraid you've forgot to attach the image…

Comment 2

[Itiel]

(In reply to Tomer Cohen :tomer from comment #1)
> (In reply to ItielMaN from comment #0)
> > Attached screenshot is the result.
> I afraid you've forgot to attach the image…

Silly me :)

Comment 3

[Itiel]

Attachment: Firefox menu button covers the entire URL bar

Comment 4

[Max Liu [:maliu]]

Attachment: Bug 1350661 - Extract layout attributes into styles in order to separate api 15 and 17 style tree,

__Device_Configuration__|__o_Applied_Style_______________
      ldrtl-v17    v17   v15 |
             o      o     o  |  UrlBar.Entry
              \     |     |  |
               -----o     |  |  UrlBar.V17.Entry(start/end)
                     \    |  |
                      \   o  |  UrlBar.V15.Entry(left/right)
                       \  |  |
                        --o  |  UrlBar.Base.Entry(original style)

Though Android support RTL since API level 17(JB_MR1), it's really buggy at that moment.
This patch fix a severe UI layout attribute bug, which only happen on android 4.2 in RTL language context:
  If view attributes "start/end" and "left/right" are both written in a view layout xml, they will both be applied and cause UI abnormal. In API 18 and above, "left" will be ignored if "start" also exist.

For example, as below show, alignLeft and alignStart are both exist in ImageView. On android 4.2 with RTL context, it's width will both align Left and Start(Right), cause the symptom that ImageView have the same width and cover on the view "back."
```
<ImageView android:id="@+id/url_bar_entry"
  android:layout_alignLeft="@+id/back"
  android:layout_alignStart="@+id/back"
```

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

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8853332 [details]
Bug 1350661 - Extract layout attributes into styles in order to separate api 15 and 17 style tree,

https://reviewboard.mozilla.org/r/125408/#review127978

::: commit-message-5182b:4
(Diff revision 1)
> +      ldrtl-v17    v17   v15 |
> +             o      o     o  |  UrlBar.Entry
> +              \     |     |  |
> +               -----o     |  |  UrlBar.V17.Entry(start/end)
> +                     \    |  |
> +                      \   o  |  UrlBar.V15.Entry(left/right)
> +                       \  |  |
> +                        --o  |  UrlBar.Base.Entry(original style)

Nice ASCII tree :)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f421e4a9970c
Extract layout attributes into styles in order to separate api 15 and 17 style tree, r=sebastian

Comment 7

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

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

Comment 8

[Itiel]

Max, can you please request an uplift to beta for this bug?

Comment 9

[Max Liu [:maliu]]

Comment on attachment 8853332 [details]
Bug 1350661 - Extract layout attributes into styles in order to separate api 15 and 17 style tree,

Approval Request Comment
[Feature/Bug causing the regression]: Fennec support RTL
[User impact if declined]: Critical impact, basic function fail on API level 17 with RTL context.
[Is this code covered by automated tests?]: No, we don't have screenshot based testing for RTL context.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, switch system locale or Fennec UI language to RTL can reproduce the symptom.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No, the solution is trivial.
[Why is the change risky/not risky?]: The solution is easy to verify.
[String changes made/needed]: None.

Comment 10

[Gerry Chang [:gchang]]

Hi Ioana, could you help find someone to verify if this RTL issue was fixed as expected on a latest Nightly build? Thanks!

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

How did we miss this issue in testing? Isn't this a major new feature for 53 Firefox for Android? 
This sounds to me like a release blocking issue for 53 Fennec.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Sounds like something we need to land for 53.  Trivial fix

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8853332 [details]
Bug 1350661 - Extract layout attributes into styles in order to separate api 15 and 17 style tree,

Take this in aurora first. Aurora54+.
Hi Mihai,
Can you help verify this on nightly & aurora?

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8853332 [details]
Bug 1350661 - Extract layout attributes into styles in order to separate api 15 and 17 style tree,

OK, let's uplift this to beta. It should be in next Monday's fennec build.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/957849c3018f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/37a857c0c3d3

Comment 17

[Itiel]

If I was the one who reported this issue in the first place, I can also confirm it is fixed in Nightly ;)
And it is fixed. Working as expected in the same device (with android 4.2.2) and the URL bar is operational.
Marking as VERIFIED FIXED.

Sorry for not confirming eariler.

Comment 18

[Ioana Chiorean]

We don't have a device with 4.2.2 that supports RTL - not sure if an addon will work. 
Thanks ItielMaN !

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> How did we miss this issue in testing? Isn't this a major new feature for 53
> Firefox for Android? 

Good question, I assume this was an uplifted regressing patch?

@Max: Is there anything we can do to prevent regressing RTL? I guess it's super easy for us front-end devs to regress RTL support without noticing. However a basic UI tests wouldn't catch that.

@No-Jun: Do you have any idea here? The first thing that came to my mind where the automated screenshots we have for Focus. We do not have anything like that for Fennec, right? Do you think it would be easy to do?

Comment 20

[Max Liu [:maliu]]

(In reply to Sebastian Kaspari (:sebastian) from comment #19)
> @Max: Is there anything we can do to prevent regressing RTL? I guess it's
> super easy for us front-end devs to regress RTL support without noticing.
> However a basic UI tests wouldn't catch that.

Sure, I thought about it when I start develop on RTL, and I did a little digging. I found this Bug 1232863 probably able to help us on prevent RTL regression. I wish this can link to a perf-herder like infrasturcture for long term monitoring. Maybe we can start to work on this bug once the build --with-gradle bug landed.

Comment 21

[No-Jun Park [:njpark]]

(In reply to Sebastian Kaspari (:sebastian) from comment #19)
> @No-Jun: Do you have any idea here? The first thing that came to my mind
> where the automated screenshots we have for Focus. We do not have anything
> like that for Fennec, right? Do you think it would be easy to do?

I wrote a tool for firefoxOS back in a while to prevent something like this: save a ref image, and run tests on nightly to compare the image to the ref (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Run_ImageMagick_Tool_with_Python_Marionette).  That part isn't hard, the difficult part would be to integrate it into treeherder. (Or if we use the qa team's jenkins server while downloading the nightly apk from taskcluster, that would be doable too) But now with kinda unexpected additional tasks, can't say for sure when I'll have a time for this, but this is now on my list.