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

()

Firefox for Android
General
VERIFIED FIXED
2 months ago
2 months ago

People

(Reporter: ItielMaN, Assigned: maliu)

Tracking

(Blocks: 1 bug, {rtl})

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

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

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 months 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.

Comment 1

2 months ago
(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 months 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 months ago
Created attachment 8851346 [details]
Firefox menu button covers the entire URL bar
Assignee: nobody → itiel_yn8
(Reporter)

Updated

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

Comment 5

2 months 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 months ago
Assignee: nobody → max

Comment 6

2 months 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
https://hg.mozilla.org/mozilla-central/rev/f421e4a9970c
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Reporter)

Comment 8

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

Comment 9

2 months 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?

Updated

2 months ago
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 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/957849c3018f
status-firefox54: affected → fixed

Comment 16

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

Comment 17

2 months 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 months 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 months 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.