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

VERIFIED FIXED in Firefox 53

Status

()

VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: itiel_yn8, Assigned: maliu)

Tracking

(Blocks: 1 bug, {rtl})

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox52 unaffected, firefox53blocking fixed, firefox54 fixed, firefox55 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
(In reply to ItielMaN from comment #0)
> Attached screenshot is the result.
I afraid you've forgot to attach the image…
(Reporter)

Comment 2

2 years ago
(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 :)
(Reporter)

Comment 3

2 years ago
Assignee: nobody → itiel_yn8
(Reporter)

Updated

2 years ago
Assignee: itiel_yn8 → nobody
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
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 :)
Attachment #8853332 - Flags: review?(s.kaspari) → review+
(Assignee)

Updated

2 years ago
Assignee: nobody → max

Comment 6

2 years ago
Pushed by max@mxli.us:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f421e4a9970c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Reporter)

Comment 8

2 years ago
Max, can you please request an uplift to beta for this bug?
Flags: needinfo?(max)
(Assignee)

Comment 9

2 years ago
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.
Flags: needinfo?(max)
Attachment #8853332 - Flags: approval-mozilla-beta?
Attachment #8853332 - Flags: approval-mozilla-aurora?
status-firefox53: --- → affected
status-firefox54: --- → affected
Hi Ioana, could you help find someone to verify if this RTL issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(ioana.chiorean)
status-firefox52: --- → unaffected
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.
tracking-firefox53: --- → blocking
Flags: needinfo?(s.kaspari)
Flags: needinfo?(rjesup)
Sounds like something we need to land for 53.  Trivial fix
Flags: needinfo?(rjesup)
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?
Flags: needinfo?(mihai.ninu)
Attachment #8853332 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8853332 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 15

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/957849c3018f
status-firefox54: affected → fixed

Comment 16

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/37a857c0c3d3
status-firefox53: affected → fixed
(Reporter)

Comment 17

2 years ago
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.
Status: RESOLVED → VERIFIED

Comment 18

2 years ago
We don't have a device with 4.2.2 that supports RTL - not sure if an addon will work. 
Thanks ItielMaN !
Flags: needinfo?(mihai.ninu)
Flags: needinfo?(ioana.chiorean)
(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?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(npark)
Flags: needinfo?(max)
(Assignee)

Comment 20

2 years ago
(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.
Flags: needinfo?(max)
(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.
Flags: needinfo?(npark)
You need to log in before you can comment on or make changes to this bug.