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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Robert Kaiser]

[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.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Important regression, tracking.

Comment 2

[Robert Kaiser]

Attachment: Screenshot: Firefox 38.0b1 fa with about:home, main menu and About dialog

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

Comment 3

[Robert Kaiser]

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.

Comment 4

[Robert Kaiser]

And actually I just did try the 38.0b1 build on Mac (Yosemite), and the About Firefox dialog works there for some reason.

Comment 5

[Axel Hecht [:Pike]]

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.

Comment 6

[Robert Kaiser]

(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?

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Axel, for the "resize to show", Windows or Linux?

Comment 8

[Axel Hecht [:Pike]]

I tested the linux 64bit builds on an ubuntu VM, running in fusion on a mac.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 10

[Axel Hecht [:Pike]]

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.

Comment 11

[Francesco Lodolo [:flod]]

I never noticed any issue with the about window running an Italian nightly build (Debian 7 running in Fusion).

Comment 12

[(no longer active)]

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.

Comment 13

[(no longer active)]

(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.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

If somebody else can get  regression window, great, otherwise I may be able to look at this tomorrow.

Comment 15

[(no longer active)]

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.  :(

Comment 16

[Alice0775 White]

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

Comment 17

[Timothy Nikkel (:tnikkel)]

(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.

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: 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.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 20

[Timothy Nikkel (:tnikkel)]

Anything quick you can see that might be causing this?

Comment 21

[Jim Mathies [:jimm]]

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.

Comment 22

[Jonathan Kew [:jfkthame]]

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.

Comment 23

[Jonathan Kew [: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.

Comment 24

[Timothy Nikkel (:tnikkel)]

Attachment: fix sprocket layout in rtl case

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?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 26

[Timothy Nikkel (:tnikkel)]

Landed with reftest
https://hg.mozilla.org/integration/mozilla-inbound/rev/d23df90fc306

Comment 27

[Timothy Nikkel (:tnikkel)]

(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.

Comment 28

[Timothy Nikkel (:tnikkel)]

Note to sheriffs: this had a full green try run. (I don't have the link handy.)

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Timothy, can we have an uplift request to aurora & beta? Thanks

Comment 30

[Timothy Nikkel (:tnikkel)]

(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.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

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?

Comment 32

[Timothy Nikkel (:tnikkel)]

(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?

Comment 33

[Jim Mathies [:jimm]]

(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.

Comment 34

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 36

[Timothy Nikkel (:tnikkel)]

(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. :)

Comment 37

[Jonathan Kew [:jfkthame]]

Do we support .xul testcases on B2G at all? I suspect it should simply be skipped.

Comment 38

[Timothy Nikkel (:tnikkel)]

Attachment: backout bug 1077085 on beta and aurora

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

Comment 39

[Timothy Nikkel (:tnikkel)]

(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.

Comment 40

[Timothy Nikkel (:tnikkel)]

Relanded with reftest disabled on mulet and b2g
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b13a16cc0e3

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a3f72dfdfac

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Whoops, landed the wrong patch on Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/830034fc6613
https://hg.mozilla.org/releases/mozilla-aurora/rev/aa08b4d5a9aa

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b13a16cc0e3

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/188117472132

Comment 45

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting as qe-verify- as this has tests. Feel free to set it back if you think this also needs some manual QA.

Comment 46

[Timothy Nikkel (:tnikkel)]

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.

Comment 47

[Cornel Ionce [:noni] [Hubs QA]]

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.