About Firefox dialog broken/empty on right to left versions of Firefox 38

VERIFIED FIXED in Firefox 38

Status

()

defect
--
blocker
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: kairo, Assigned: tnikkel)

Tracking

({regression})

unspecified
mozilla40
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox37 unaffected, firefox38+ verified, firefox39 verified, firefox40 verified, b2g-v2.2 unaffected, b2g-master fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
[Tracking Requested - why for this release]:

I saw our automated tests of Firefox update being broken in 38.0b1 for fa locale yesterday.

Today, I did some more digging into it, including doing some manual testing, I saw that what happens is this:

1) The test successfully goes to the "About Firefox" dialog in the old beta and the update to 38.0b1 downloads and installs.
2) After restarting into 38.0b1, it goes to the "About Firefox" dialog again to verify that no further update is found and *times out*.
3) This happens on Linux and Windows at least, Mac seems to be fine for some reason.

On manually checking, I found that at least on Linux, the "About Firefox" dialog on 38.0b1 comes up completely blank, without any content at all.

(Note that next to that, 38 has even more en-US text spread into main screens like about:home, main menu and help submenu, than the 36 beta I used for trying to start the update. The whole locale seems to nowadays be a very bad to look at mixture of halfway RTL-ized en-US text and Persian localized text. But that's a general observation and not the core of this bug.)

With this state of the "About Firefox" window, I do not see myself able to sign-off from the QA side on releasing Persian Firefox 38 to the beta population.
Assignee: nobody → sara.mansouri
Important regression, tracking.
(Reporter)

Comment 2

4 years ago
Here's a screenshot that shows Fireofx 38.0b1 fa with about:home, the main menu, and the empty "About Firefox" dialog on Linux.
(Reporter)

Comment 3

4 years ago
On the Windows/Linux vs. Mac thing, note that I'm not sure if our update tests use the same method to trigger manual updates on Mac, due to differences in the UI on OS X. Possibly they might not use the About dialog there, I'm not sure.
(Reporter)

Comment 4

4 years ago
And actually I just did try the 38.0b1 build on Mac (Yosemite), and the About Firefox dialog works there for some reason.
Same problem on aurora, fwiw.

Note, I got this fixed by just resizing the about window a tiny bit, then the content shows.

Doubt this is actually a problem with the persian locale per se.
(Reporter)

Comment 6

4 years ago
(In reply to Axel Hecht [:Pike] from comment #5)
> Note, I got this fixed by just resizing the about window a tiny bit, then
> the content shows.
> 
> Doubt this is actually a problem with the persian locale per se.

Interesting (though no idea why it apparently only happens for fa in this case).
Milan, might this be a gfx issue?
Flags: needinfo?(milan)
Axel, for the "resize to show", Windows or Linux?
Flags: needinfo?(milan) → needinfo?(l10n)
I tested the linux 64bit builds on an ubuntu VM, running in fusion on a mac.
Flags: needinfo?(l10n) → needinfo?(milan)
Thanks - in the meantime, we reproduced it on Linux directly.  Happens with any other RTL languages?  Or LTR languages for that matter?  Do we have a way of running mozregression on localized builds or figuring out when and how this broke?
There seems to be a line along the right edge of the about window before resizing, I wonder if that's a clue.
Flags: needinfo?(milan)
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ only has builds going back a few days. Directories are ending in -mozilla-aurora-l10n.

You can also test the 'he' builds, Hebrew, which gives you another RTL locale.

I don't think one can run mozregression on it without heavily patching things. "Heavily" being:

Download the nightly, extract the revision, update a corresponding hg repo to that revision, do a l10n repack of that nightly with l10n merge, extract the binary from the generated installer, run the test.
I never noticed any issue with the about window running an Italian nightly build (Debian 7 running in Fusion).
This also happens with Hebrew and Arabic, and also with an en-US build of 38 if you set the intl.uidirection.en pref to "rtl" in order to turn the UI to RTL, so it is not related to the Persian locale.  Moving to Graphics for now.
Assignee: sara.mansouri → nobody
Component: fa / Persian → Graphics
Product: Mozilla Localizations → Core
Summary: About Firefox dialog broken/empty on fa locale in 38 Beta 1 → About Firefox dialog broken/empty on right to left versions of Firefox 38
(In reply to Axel Hecht [:Pike] from comment #10)
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ only has builds
> going back a few days. Directories are ending in -mozilla-aurora-l10n.
> 
> You can also test the 'he' builds, Hebrew, which gives you another RTL
> locale.
> 
> I don't think one can run mozregression on it without heavily patching
> things. "Heavily" being:
> 
> Download the nightly, extract the revision, update a corresponding hg repo
> to that revision, do a l10n repack of that nightly with l10n merge, extract
> the binary from the generated installer, run the test.

Setting the intl.uidirection.en pref to "rtl" should be enough for the purposes of running mozregression.
If somebody else can get  regression window, great, otherwise I may be able to look at this tomorrow.
Regression range: <https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=48171dc1&tochange=efe76955>

Likely cause: bug 1116588.

Kats, can you take a look, please?  The l10n is considering to pull the plug on Persian (and perhaps other RTL locales) Firefox in 38 because of this regression.  :(
Assignee: nobody → bugmail.mozilla
Blocks: 1116588
Component: Graphics → Layout

Comment 16

4 years ago
Using force-RTL 2.1 addon (it is necessary to disable e10s to prevent crash)

Pushlog: 
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4368f3945690&tochange=95c76c3b0172

Suspect: f80d28a719c0 Mark Capella — Bug 1123606 - Fix RTL Logic error affecting Handle Movement in TextAreas, r=margaret
(Assignee)

Comment 17

4 years ago
(In reply to Alice0775 White from comment #16)
> Using force-RTL 2.1 addon (it is necessary to disable e10s to prevent crash)
> 
> Pushlog: 
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=4368f3945690&tochange=95c76c3b0172
> 
> Suspect: f80d28a719c0 Mark Capella — Bug 1123606 - Fix RTL Logic error
> affecting Handle Movement in TextAreas, r=margaret

That's android only. So I'm guessing bug 1077085.
(Assignee)

Updated

4 years ago
Assignee: bugmail.mozilla → nobody
No longer blocks: 1116588
(Assignee)

Updated

4 years ago
Blocks: 1077085
Assignee: nobody → jmathies
(Assignee)

Comment 18

4 years ago
Posted file frametree
This appears to be a reflow bug. Resizing the about dialog window fixes the problem.

0x7fffc23697e8 contains the entire page. It's positioned at 44220,0. The viewport frame has size 44280,17460. So 0x7fffc23697e8 is almost entirely positioned out of view.
The window is not available but it does not break the update scenario.
I don't think we are going to block the 38 beta 1 push because of this bug.
(Assignee)

Comment 20

4 years ago
Anything quick you can see that might be causing this?
Flags: needinfo?(smontagu)
Flags: needinfo?(jfkthame)
I wonder if this window falls into the !mAttachedToParent check we added in that patch? If it normally fell through to SetWindowDimensions maybe we generate paints more reliably in that path.
When this code is called, mAttachedToParent is true, so prior to bug 1077085 neither the mWindow->Resize() nor the mViewManager->SetWindowDimensions() here gets called: we took the if-branch of the outer "(mWindow)" condition, but then failed the inner "(!mAttachedToParent)" test.

After bug 1077085, these two tests are combined in a single if(), which fails, so now we do get to the SetWindowDimensions() call. And that seems to result in the failure here.

I don't know quite why that causes a problem, but it looks like the RTL content is being placed incorrectly because it is dependent on knowing the "container width" within which reflow is happening in order to convert logical (RTL, origin at right) to physical (LTR, origin at left) coordinates.

Perhaps the call to SetWindowDimensions() here is causing us to reflow the content of the window too soon, before its overall size is properly set, and so we're doing those conversions based on an incorrect container width.
Flags: needinfo?(jfkthame)
(In reply to Timothy Nikkel (:tn) from comment #18)
> Created attachment 8587289 [details]
> frametree
> 
> This appears to be a reflow bug. Resizing the about dialog window fixes the
> problem.
> 
> 0x7fffc23697e8 contains the entire page. It's positioned at 44220,0. The
> viewport frame has size 44280,17460. So 0x7fffc23697e8 is almost entirely
> positioned out of view.

Interestingly, when opening the about dialog, we appear to reflow its contents twice. The first time, it looks like we may be getting it correct:

Viewport(-1)@7fffbd8c6458 [view=7fffb34aa900] {0,0,43440,17340} [state=000b063000042220] [sc=7fffbd8c60f8:-moz-viewport^0]<
  RootBox(window)(-1)@7fffbd8c6e00 {0,0,43440,17340} [state=000b060081c40000] [content=7fffb09978f0] [sc=7fffbd8c6910:-moz-canvas]<
    DocElementBox(window)(-1)@7fffbd8c7120 {0,0,43440,17340} [state=000b160081a40000] [content=7fffb09978f0] [sc=7fffbd8c6f68^0]<
      PopupSet(popupgroup)(-1)@7fffbdae1020 next=7fffbdae1410 {0,0,43440,0} [state=0009160000c40008] [content=7fffb32fb590] [sc=7fffbd8c7298]
      PopupList 7fffbdae10c0 <
        MenuPopup(tooltip)(-1)@7fffbdae10d0 [view=7fffb34aaf20] {0,0,0,0} [state=000d06108080250a] [content=7fffb32fb620] [sc=7fffbd8c78b0^7fffbd8c6f68^0]<>
      >
      Placeholder(tooltip)(-1)@7fffbdae1410 next=7fffbdae1508 {0,0,43440,0} [state=0009000000800000] [content=7fffb32fb620] [sc=7fffbdae11f8:-moz-non-element] outOfFlowFrame=MenuPopup(tooltip)(-1)@7fffbdae10d0
      Box(vbox)(1)@7fffbdae1508 {0,0,43440,17340} [state=000b160080800200] [content=7fffb0997aa0] [sc=7fffbd8c7e08]<

but then we reflow again and that Box(vbox) ends up way over on the right:

Viewport(-1)@7fffbd8c6458 [view=7fffb34aa900] {0,0,43440,17340} [state=000b063000042220] [sc=7fffbd8c60f8:-moz-viewport^0]<
  RootBox(window)(-1)@7fffbd8c6e00 {0,0,43440,17340} [state=000b060081c40000] [content=7fffb09978f0] [sc=7fffbd8c6910:-moz-canvas]<
    DocElementBox(window)(-1)@7fffbd8c7120 {0,0,43440,17340} [state=000b160081a40000] [content=7fffb09978f0] [sc=7fffbd8c6f68^0]<
      PopupSet(popupgroup)(-1)@7fffbdae1020 next=7fffbdae1410 {43380,0,43440,0} [state=0009160000c40008] [content=7fffb32fb590] [sc=7fffbd8c7298]
      PopupList 7fffbdae10c0 <
        MenuPopup(tooltip)(-1)@7fffbdae10d0 [view=7fffb34aaf20] {0,0,0,0} [state=000d06108080250a] [content=7fffb32fb620] [sc=7fffbd8c78b0^7fffbd8c6f68^0]<>
      >
      Placeholder(tooltip)(-1)@7fffbdae1410 next=7fffbdae1508 {43380,0,43440,0} [state=0009000000800000] [content=7fffb32fb620] [sc=7fffbdae11f8:-moz-non-element] outOfFlowFrame=MenuPopup(tooltip)(-1)@7fffbdae10d0
      Box(vbox)(1)@7fffbdae1508 {43380,0,43440,17340} [state=000b160080800200] [content=7fffb0997aa0] [sc=7fffbd8c7e08]<

This still looks to me like it could result from a wrong container-width at some level, though I'm not sure where/when.
(Assignee)

Comment 24

4 years ago
This is a very foolish patch to propose. I don't think there have been any functional changes to this code since 2000.

What do you think roc? Am I just waking up sleeping dragons who will burn down my house in the night?
Flags: needinfo?(smontagu)
Attachment #8588525 - Flags: review?(roc)
Comment on attachment 8588525 [details] [diff] [review]
fix sprocket layout in rtl case

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

A reftest would be good.

Changing this is scary, but this seems OK.

One option is to let this ride the trains and instead of landing this on beta, convert the affected XUL to HTML + CSS flexbox instead.
Attachment #8588525 - Flags: review?(roc) → review+
(Assignee)

Comment 26

4 years ago
Landed with reftest
https://hg.mozilla.org/integration/mozilla-inbound/rev/d23df90fc306
Assignee: jmathies → tnikkel
(Assignee)

Comment 27

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> One option is to let this ride the trains and instead of landing this on
> beta, convert the affected XUL to HTML + CSS flexbox instead.

I was planning on letting it ride the trains. Gavin, how do you feel about rewriting the about dialog in HTML and CSS flexbox and uplifting to beta? If not we can limit the patch from bug 1077085 (which exposed this bug) to content processes, which is the only place that the fix from bug 1077085 should be important.
Flags: needinfo?(gavin.sharp)
(Assignee)

Comment 28

4 years ago
Note to sheriffs: this had a full green try run. (I don't have the link handy.)
Timothy, can we have an uplift request to aurora & beta? Thanks
Flags: needinfo?(tnikkel)
(Assignee)

Comment 30

4 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #29)
> Timothy, can we have an uplift request to aurora & beta? Thanks

I would not like to uplift this patch to aurora or beta. We have two options for uplift outlined in comment 27. I'm waiting to hear back from Gavin about the more desirable option. If not I can cook up a patch for the second option.
Flags: needinfo?(tnikkel)
OK! We would like a patch asap as it is blocking all RTL updates.
How long would take a rewrite of the about page?

Btw, why don't we just backout bug 1077085?
(Assignee)

Comment 32

4 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #31)
> Btw, why don't we just backout bug 1077085?

Good idea, I hadn't thought about this. If the fix is only needed for e10s we shouldn't need it on aurora or beta I would guess. Jim, any reason not to do that?
Flags: needinfo?(jmathies)
(In reply to Timothy Nikkel (:tn) from comment #32)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #31)
> > Btw, why don't we just backout bug 1077085?
> 
> Good idea, I hadn't thought about this. If the fix is only needed for e10s
> we shouldn't need it on aurora or beta I would guess. Jim, any reason not to
> do that?

Fine with me assuming we're keeping the fix on mc.
Flags: needinfo?(jmathies)
OK, let's do it then :) Timothy, could you take care of that for 38 & 39? Thanks

+ adding the appropriate flag to make sure that it is verified on mc.
Flags: qe-verify+
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tn) from comment #28)
> Note to sheriffs: this had a full green try run. (I don't have the link
> handy.)

Backed out for frequent B2G reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/07ca9739edf0

https://treeherder.mozilla.org/logviewer.html#?job_id=8524610&repo=mozilla-inbound
(Assignee)

Comment 36

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #35)
> (In reply to Timothy Nikkel (:tn) from comment #28)
> > Note to sheriffs: this had a full green try run. (I don't have the link
> > handy.)
> 
> Backed out for frequent B2G reftest failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/07ca9739edf0
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=8524610&repo=mozilla-
> inbound

Oops. The try server push only had the patch as I hadn't written the reftest yet. :)
Do we support .xul testcases on B2G at all? I suspect it should simply be skipped.
(Assignee)

Comment 38

4 years ago
Approval Request Comment
[Feature/regressing bug #]: bug 1077085
[User impact if declined]: about dialog in RTL will be blank on some systems
[Describe test coverage new/current, TreeHerder]: I have a reftest to land with the proper fix
[Risks and why]: safest option, backout a patch only needed for e10s
[String/UUID change made/needed]: none
Flags: needinfo?(tnikkel)
Flags: needinfo?(gavin.sharp)
Attachment #8589454 - Flags: approval-mozilla-beta?
Attachment #8589454 - Flags: approval-mozilla-aurora?
Attachment #8589454 - Flags: approval-mozilla-beta?
Attachment #8589454 - Flags: approval-mozilla-beta+
Attachment #8589454 - Flags: approval-mozilla-aurora?
Attachment #8589454 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 39

4 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #37)
> Do we support .xul testcases on B2G at all? I suspect it should simply be
> skipped.

Yes of course you are right.
(Assignee)

Comment 40

4 years ago
Relanded with reftest disabled on mulet and b2g
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b13a16cc0e3
https://hg.mozilla.org/mozilla-central/rev/9b13a16cc0e3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Setting as qe-verify- as this has tests. Feel free to set it back if you think this also needs some manual QA.
Flags: qe-verify+ → qe-verify-
(Assignee)

Comment 46

4 years ago
The tests are only on m-c because we only fixed the root issue on m-c. The tests would fail if we landed them on aurora or beta. We landed a different patch on aurora and beta.
Flags: qe-verify- → qe-verify+
Reproduced the initial issue using 38.0b1 (fa/he/ar builds).

Confirming the fix on Ubuntu 14.04 64-bit using the fa/he/ar/en-us (intl.uidirection.en set to rtl) with:
- latest Nightly, build ID: 20150419030206;
- latest Aurora, build ID: 20150419004004;
- Firefox 38 beta 5, build ID: 20150416143048.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
You need to log in before you can comment on or make changes to this bug.