This has originally been found while investigating on bug 1119420 and bug 1119381. Here’s a way to reproduce it: • Settings > Language > Arabic • long-press the power button Expected: the power menu appears. Actual: the menu is blank. It seems that in some cases, Gecko does not render elements any more in RTL blocks (everything is fine in LTR). It looks like static elements are displayed properly, but dynamically created/modified elements can be completely blank. When inspecting such elements in the WebIDE, everything gets back to normal. Here’s a rough regression window, using the latest Gaia on my Keon: • Gecko-7b34913: works (GeeksPhone 2015-01-07 build) • Gecko-58c53ce: buggy (GeeksPhone 2015-01-09 build) This is a blocker for many RTL-related items.

I currently have Bug 1119420, Bug 1119380, Bug 1119381, Bug 1119371 occurring in Firefox OS 2.2 and master (currently 3.0) The common thing between all those bugs is that, they all appeared at once in the build of 7 Jan(1), meaning that in the 6th Jan build(2) they were not there. As Kaze says, best way to reproduce this is • Settings > Language > Arabic • long-press the power button The bug is about elements in Gaia disappearing when the system is set to a RTL-based language i.e. Arabic. So I went further and tried both builds again (and keeping the same gaia revision), the bug did not occur in build (2) but did in build (1) So the conclusion is that, no gaia code is causing this, but more likely gecko. My regression window is gecko mozillaorg 7b34913ca740dc6bceab66c69e26b0e860c250ba WORKING gecko mozillaorg 0cf53b165b6b664939ba8c2834c9b869c83a82e7 BUGGY (1): http://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/2015-01-07-16-02-20-mozilla-central-flame-kk/ (2): http://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/2015-01-06-16-02-03-mozilla-central-flame-kk/

Kats, do you think this could be related to bug 1109873 ? (very wild guess)

Also, even though the title says RTL, this is not a RTL-specific bug (it's not even a Firefox OS bug). This seems like a gecko bug, which means *websites* out there could get affected by it. Only reason RTL is mentioned here is that we don't have an explanation for it yet, and the easiest and most obvious way to reproduce it is by using Firefox OS in RTL and following the steps in Comment 1.

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #1) > So I went further and tried both builds again (and keeping the same gaia > revision), the bug did not occur in build (2) but did in build (1) > > So the conclusion is that, no gaia code is causing this, but more likely > gecko. > My regression window is > gecko mozillaorg 7b34913ca740dc6bceab66c69e26b0e860c250ba WORKING > gecko mozillaorg 0cf53b165b6b664939ba8c2834c9b869c83a82e7 BUGGY > > > (1): > http://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/2015-01-07-16-02-20- > mozilla-central-flame-kk/ > (2): > http://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/2015-01-06-16-02-03- > mozilla-central-flame-kk/ If I'm converting this right, this corresponds to the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b42615e51c81&tochange=70de2960aa87 which has a lot of commits. I'm not sure why you guessed bug 1109873 but it seems unlikely to me that it would affect RTL rendering (also, it's not in this regression range). I think the best thing to do here would be to request a regression window to narrow down the range of possibilities using central and inbound/b2g-inbound builds.

Also cc'ing Jonathan Kew who would have a better idea of which changes in the range in comment 4 might have affected this. As you said in comment 3 it might not be limited to RTL but it's a good place to start.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4) > I'm not sure why you guessed bug 1109873 but it seems unlikely to me that > it would affect RTL rendering (also, it's not in this regression range). Sorry, there’s been a misunderstanding on our side. Working too late generates bad conclusions. Thanks for pinging back and for cc’ing Jonathan!

Taking a wild guess based on the range from comment 4: this might possibly have been triggered by bug 1079154. Maybe someone can confirm/deny that using inbound builds? It would be awesome if we could get a testcase (maybe an HTML page based on the affected Gaia code) that exhibits the Gecko bug "standalone" in a browser, without requiring a FxOS build.

Comment 7 guessed right. mozilla-inbound regression window: Last Working Environmental Variables: Device: Flame BuildID: 20150106230728 Gaia: 69ac77cfa938fae2763ac426a80ca6e5feb6ad25 Gecko: 13869ca774bb Version: 37.0a1 (2.2 Master) Firmware: V18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0 First Broken Environmental Variables: Device: Flame BuildID: 20150106231227 Gaia: 69ac77cfa938fae2763ac426a80ca6e5feb6ad25 Gecko: 547a2d626a62 Version: 37.0a1 (2.2) Firmware: V18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0 Gaia is the same so it's a Gecko issue. Gecko pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=13869ca774bb&tochange=547a2d626a62 Caused by patches for bug 1079154.

Thank you all for your help! Simon, Jonathan, here’s a minified test case: occurs in today’s Nightly (Linux64 w/ e10s), not with Aurora.

Changing the product/component, as this affects the desktop browser as well. This is a serious regression for web content RTL support on both Firefox and Firefox OS.

Reproduces on OSX also

This fixes the minimized testcase. Can someone test whether it fixes all the Gaia issues? There are try builds at https://ftp-ssl.mozilla.org/pub/mozilla.org/b2g/try-builds/smontagu@mozilla.com-30e6d701934c/ -- please let me know if I need to do a try push with other options to produce useful builds for testing.

Thanks for the quick fix simon. I'm AFK at the moment. Ahmed, can you check if this fixes our RTL beasts please?

I just tried what's compiled from the link in comment 12 and unfortunately it does not fix the gaia issue

I can't reproduce all the Gaia regressions with the desktop emulator, but I do see bug 1119371, and this patch fixes that. (I tried a similar approach to the previous patch, but that didn't fix the regression. If this is sufficient to fix all the problems with Gaia, there shouldn't be any problem with deferring the patch to nsFlexContainerFrame from bug 1079154 until after bug 1079155)

Thanks for testing, Ahmed. Can you try with the patch in comment 15? Try builds with both patches will be at https://ftp-ssl.mozilla.org/pub/mozilla.org/b2g/try-builds/smontagu@mozilla.com-bca1adb6b924 when they're done.

(In reply to Simon Montagu :smontagu from comment #16) > Thanks for testing, Ahmed. Can you try with the patch in comment 15? Try > builds with both patches will be at > https://ftp-ssl.mozilla.org/pub/mozilla.org/b2g/try-builds/smontagu@mozilla. > com-bca1adb6b924 when they're done. I'm sorry Simon but that doesn't seem to fix it :( bug 1119371 along the others are still reproducing..

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #17) > (In reply to Simon Montagu :smontagu from comment #16) > > Thanks for testing, Ahmed. Can you try with the patch in comment 15? Try > > builds with both patches will be at > > https://ftp-ssl.mozilla.org/pub/mozilla.org/b2g/try-builds/smontagu@mozilla. > > com-bca1adb6b924 when they're done. > > I'm sorry Simon but that doesn't seem to fix it :( bug 1119371 along the > others are still reproducing.. Comment 15 said that the patch fixes 1119371, but comment 17 says it doesn't. Some difference in testing methodology? I tried to reproduce bug 1119371 on desktop Nightly using the FxOS 2.2 simulator, but I can't seem to do so with any reliability. Once or twice, I did get a fleeting glimpse of mis-spaced status bar icons, but I can't reproduce on demand, and it quickly corrected itself. Any hints on how to consistently reproduce the problem for testing purposes?

(In reply to Jonathan Kew (:jfkthame) from comment #18) > Comment 15 said that the patch fixes 1119371, but comment 17 says it > doesn't. Some difference in testing methodology? > > I tried to reproduce bug 1119371 on desktop Nightly using the FxOS 2.2 > simulator, but I can't seem to do so with any reliability. Once or twice, I > did get a fleeting glimpse of mis-spaced status bar icons, but I can't > reproduce on demand, and it quickly corrected itself. Any hints on how to > consistently reproduce the problem for testing purposes? For the status bar you only have to go in and out couple of apps. The two other obvious cases are accessing power menu (which do not appear at all unless a reflow is triggered, e.g. changing volume) and pulling down notification tray.

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #19) > The two other obvious cases are accessing power menu (which do not appear at > all unless a reflow is triggered, e.g. changing volume) and pulling down > notification tray. How can one get to the power menu on the desktop simulator? I see how to pull down the notification tray, but how can I send myself some notifications?

(In reply to Simon Montagu :smontagu from comment #20) > (In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #19) > > > The two other obvious cases are accessing power menu (which do not appear at > > all unless a reflow is triggered, e.g. changing volume) and pulling down > > notification tray. > > How can one get to the power menu on the desktop simulator? I see how to > pull down the notification tray, but how can I send myself some > notifications? for notifications, calendar events reminders would do it power menu, i myself not sure how to trigger that anymore

This is still a worrisome issue and blocking RTL. Can we get this as blocker for 2.2 so it gets more TLC? thanks!

(In reply to Delphine Lebédel [:delphine - use need info] from comment #22) > This is still a worrisome issue and blocking RTL. Can we get this as blocker > for 2.2 so it gets more TLC? > thanks! We're treating it as a blocker now. :smontagu is going to back out the regressing patch in 2.2 branch. We're going to keep working on a fix on trunk.

(In reply to Jet Villegas (:jet) from comment #23) > (In reply to Delphine Lebédel [:delphine - use need info] from comment #22) > > This is still a worrisome issue and blocking RTL. Can we get this as blocker > > for 2.2 so it gets more TLC? > > thanks! > > We're treating it as a blocker now. :smontagu is going to back out the > regressing patch in 2.2 branch. We're going to keep working on a fix on > trunk. Thanks Jet!

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): 1079154 User impact if declined: UI misrendered in RTL languages Testing completed: Tested on desktop simulator. Risk to taking this patch (and alternatives if risky): Returns to the status quo before bug 1079154 (which adds support for some aspects of vertical text layout, a feature not supported for Gaia2.2 in any case) String or UUID changes made by this patch: None

See bug 1125808 for a parallel regression in web content (misplaced content rather than disappearing) at http://www.alquds.com/news/article/view/id/537890

Comment on attachment 8551138 [details] [diff] [review] Patch Simon - You commented in bug 1125808 that this patch fixes the issue reported on desktop as well. If this is the case, can you please request Aurora uplift for this patch?

Jet - In Comment 23 you stated that the layout team is working on a proper fix on trunk. Can we set a date by which a proper fix is needed or the back out will be applied on trunk so as not to have this bug merge to Aurora again in 38?

Comment on attachment 8554444 [details] [diff] [review] Branch patch: back out all the checkins from bug 1079154 Yes, even though we didn't discuss backing out from Aurora earlier, it obviously makes sense by the same logic we used for Gaia2.2: bug 1079154 considered in isolation doesn't add value on Aurora since vertical text isn't supported there anyway, so backing out is the minimum-risk fix for the regressions. Approval Request Comment [Feature/regressing bug #]: 1079154 [User impact if declined]: Elements misplaced or invisible in RTL web content (see bug 1125808 for an example, but there are likely to be other instances) [Describe test coverage new/current, TreeHerder]: Known regressions tested and confirmed as fixed. [Risks and why]: Minimal risk as explained above [String/UUID change made/needed]: None

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #29) > Jet - In Comment 23 you stated that the layout team is working on a proper > fix on trunk. Can we set a date by which a proper fix is needed or the back > out will be applied on trunk so as not to have this bug merge to Aurora > again in 38? The nature of the bug (RTL + flexbox) makes this a blocker for B2G 2.2 but a lot less severe for external web content. We're working on this as top-priority and not let it ride to Aurora, but I don't think backing it out of trunk is necessary.

Leaving bug 1079154 in master makes RTL development for B2G extremely difficult since we do all our work on master and then uplift to 2.2. As it stands, we can't effectively test on master. If this cannot be fixed in the next week, please consider backing out bug 1079154 on master and then re-landing it with a fix to this bug.

Comment on attachment 8554444 [details] [diff] [review] Branch patch: back out all the checkins from bug 1079154 Aurora+ for this backout.

https://hg.mozilla.org/releases/mozilla-aurora/rev/a96c54ab12ff For future reference, asking "Does this affect Aurora too?" up front would have saved a round of approvals and uplifts for everyone since Aurora37 merges to b2g37 during this cycle.

Unlike attachment 8552016 [details] [diff] [review], this aims to correct the buggy code in nsFlexContainerFrame rather than back it out. Attachment 8551138 [details] [diff] and this patch together fix all the problems that I can reproduce on the desktop simulator. Jonathan, can you test them on an actual device?

(In reply to Simon Montagu :smontagu from comment #36) > Attachment 8551138 [details] [diff] and this patch together fix all the > problems that I can reproduce on the desktop simulator. I do see some overlapping elements in the status bar in Notifications, but that seems to be a different issue because I see it in LTR languages too.

That seems to have been bug 1115172: I no longer see it after updating Gaia.

Comment on attachment 8558141 [details] [diff] [review] Patch 2 v.2: make nsFlexContainerFrame update container width and frame width before using them in ApplyRelativePositioning and FinishReflowChild Review of attachment 8558141 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good so far; in testing on my Peak device, it seems to fix the flexbox-related RTL issues I was seeing. I'm seeing one remaining statusbar issue, but I suspect it may be a Gaia CSS problem rather than a platform bug; still trying to pin it down to confirm this one way or the other. I've also put together a couple of reduced testcases in reftest form, one for each of the two patches. These are tests that fail on desktop Firefox without the patches here, and are fixed when they are applied. I'll attach these in a moment. ::: layout/generic/nsFlexContainerFrame.cpp @@ +3888,5 @@ > + // Update the container width and frame width before using them in > + // ApplyRelativePositioning and FinishReflowChild > + aContainerWidth -= aItem.Frame()->GetSize().width; > + aItem.Frame()->SetSize(wm, childDesiredSize.Size(wm)); > + aContainerWidth += aItem.Frame()->GetSize().width; I wonder if it would be clearer to write this a bit differently: rather than the two-step modification of aContainerWidth, how about something like this? nscoord dw = childDesiredSize.Width() - aItem.Frame()->GetSize().width; aContainerWidth += dw; That should be equivalent, afaics, but I think it makes it easier to understand what's being done to aContainerWidth here. I'm still a little uneasy, though, about patching in quite this way... it feels rather hackish. The underlying problem seems to be that we converted a physical position (determined by the flexbox code) to logical aFramePos using the frame's "old" width, which means it's wrong in the case of RTL direction and a change to the frame width. Then we're tweaking aContainerWidth here to compensate for that. Would it be feasible instead to use the correct width when initially computing the frame's logical position? If we use finalFlexedPhysicalSize.width instead of the frame's Size().width when converting physicalPosn to (logical) framePos in DoFlexLayout, I think we can avoid the ugliness here. Will test some more....

Desktop reftest that depends on the first patch in this bug (derived from :kaze's testcase in attachment 8549858 [details]). Note that this does not involve flexbox at all, just block reflow and relative positioning.

And here's one involving flexbox that depends on patch 2v2.

Here's an alternative version of patch 2; AFAICS this should achieve an equivalent outcome, but with a more direct approach to the positioning computation; see what you think.

Comment on attachment 8551138 [details] [diff] [review] Patch Marking this r+, as we need this patch regardless of exactly which approach we take to the flex-container issue; really, there are two distinct (though very similar) bugs we're fixing here, as illustrated by the two independent reftests.

Try job with the alternative patch 2 (attachment 8558528 [details] [diff] [review]), as well as the original patch here, and the two reftests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc8d5ad00a2c.

(In reply to Jonathan Kew (:jfkthame) from comment #40) > ::: layout/generic/nsFlexContainerFrame.cpp > @@ +3888,5 @@ > > + // Update the container width and frame width before using them in > > + // ApplyRelativePositioning and FinishReflowChild > > + aContainerWidth -= aItem.Frame()->GetSize().width; > > + aItem.Frame()->SetSize(wm, childDesiredSize.Size(wm)); > > + aContainerWidth += aItem.Frame()->GetSize().width; > > I wonder if it would be clearer to write this a bit differently: rather than > the two-step modification of aContainerWidth, how about something like this? > > nscoord dw = childDesiredSize.Width() - aItem.Frame()->GetSize().width; > aContainerWidth += dw; > > That should be equivalent, afaics, but I think it makes it easier to > understand what's being done to aContainerWidth here. I was hesitating between these two alternatives myself, and wasn't sure which one was clearer. I'm happy to go the other way if you prefer it. > I'm still a little uneasy, though, about patching in quite this way... it > feels rather hackish. The underlying problem seems to be that we converted a > physical position (determined by the flexbox code) to logical aFramePos > using the frame's "old" width, which means it's wrong in the case of RTL > direction and a change to the frame width. Then we're tweaking > aContainerWidth here to compensate for that. Here I disagree with you, but maybe I'm misunderstanding what the code is doing. My interpretation was that we were calculating the correct position of the frame before this reflow, but when calculating the position after reflow with the frame's new size, the old value of aContainerWidth was wrong because the container needed to expand to fit the frame. Your approach seems to me the hacky one because it calculates the "old" frame position with the "new" frame size, and then that gets compensated for by calling ApplyRelativePositioning with the "old" size. But as I say, I'm not really sure if all that is correct or based on faulty premises.

(In reply to Simon Montagu :smontagu from comment #46) > Here I disagree with you, but maybe I'm misunderstanding what the code is > doing. My interpretation was that we were calculating the correct position > of the frame before this reflow, but when calculating the position after > reflow with the frame's new size, the old value of aContainerWidth was wrong > because the container needed to expand to fit the frame. AIUI, that's not how things work here. By this time, the flexbox container code has computed the final (possibly "flexed") size and position of all its items -- note the comment above the loop: // FINAL REFLOW: Give each child frame another chance to reflow, now that // we know its final size and position. These sizes and positions are stored as physical values in the "main" and "cross" size and position fields of the FlexItem. So for RTL items, the "position" we get from the FlexItem will be the physical (top-left) origin where the child frame is to be placed. To convert this to a logical-coord position, we need to use the *final* width that the frame's going to have, even if it hasn't yet been set to that size. The other thing that I think highlights the hackiness of the other patch is that it's messing with aContainerWidth on a per-child basis, whereas the "true" container width for placement of all the flex-item children should be the same: it's the flex container that holds them. Note how the same containerWidth is passed to ReflowFlexItem for each item in the loop; it doesn't make sense to me that each ReflowFlexItem then modifies that containerWidth internally before passing it to ApplyRelativePositioning and FinishReflowChild, as the container is not in fact being modified at this stage. This is the "hacky" compensation for having passed in the wrong (logical) desired position for the child, because the (correct) physical position was converted based on a non-final width. Does that make sense, or have I just stirred the mud?

Comment on attachment 8558528 [details] [diff] [review] patch 2 (alternative) - Use the flex-item frame's final size when computing its logical position within the flex container Review of attachment 8558528 [details] [diff] [review]: ----------------------------------------------------------------- Yes, comment 47 makes sense, let's do it this way.

https://hg.mozilla.org/integration/mozilla-inbound/rev/79d17c017fb6 https://hg.mozilla.org/integration/mozilla-inbound/rev/e17c2405c2cd https://hg.mozilla.org/integration/mozilla-inbound/rev/c460e492a4bd https://hg.mozilla.org/integration/mozilla-inbound/rev/1b096ee268cb

https://hg.mozilla.org/mozilla-central/rev/79d17c017fb6 https://hg.mozilla.org/mozilla-central/rev/e17c2405c2cd https://hg.mozilla.org/mozilla-central/rev/c460e492a4bd https://hg.mozilla.org/mozilla-central/rev/1b096ee268cb

This issue has been verified successfully on Flame 2.2 Reproduce rate:0/5 Attachment:Verify_RTL_Power.png Flame 2.2 build: Build ID 20150204162500 Gaia Revision c2047a46e29696238e9b4c9caaba47736421449a Gaia Date 2015-02-04 20:34:04 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/adfba0a07e9b Gecko Version 37.0a2 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150204.195445 Firmware Date Wed Feb 4 19:54:56 EST 2015 Bootloader L1TC000118D0

Still have a little element which is going out of the screen on the current master: turn on your flame without any simcard inside and pull down notification menu, the message for the missing simcard is a bit out of the screen on the left.

(In reply to Clément Lefèvre from comment #54) > Still have a little element which is going out of the screen on the current > master: turn on your flame without any simcard inside and pull down > notification menu, the message for the missing simcard is a bit out of the > screen on the left. That sounds like bug 1128981; I've posted a patch that is awaiting review.

This issue is verified fixed on Flame Master. Result: Text and visual elements are displayed correctly on Settings, power menu, and notification tray. Device: Flame Master (319mb, full flash) Build ID: 20150205010209 Gaia: 2b83a6d5d1185a438b5bbd287497ac2743b501db Gecko: 34a66aaaca81 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 38.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0