477754 - [RTL] Customize toolbar sheet is placed mostly outside of the browser

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Wrongly positioned customize toolbar sheet

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; he; rv:1.9.1b3pre) Gecko/20090207 Shiretoko/3.1b3pre

Calling the customize toolbar sheet via the context menu of the toolbar, opens the sheet mostly outside of the current browser window. See the attached screenshot.

Steps:
1. Start Firefox
2. Open Context menu of the navigation toolbar
3. Click on Customize

No idea if this only happens on OS X. Someone should verify this topic on Windows or Linux too. I believe that is a regression, or?

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Regression range?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The regression range is somewhere before FF3.0b3. Means back in 2007. The latest build I was able to test is 08010104. Builds from 2007 don't work with the force locale extension. I don't have time right now to get an exact time frame for rtl locales.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

And as a side note. With Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.20) Gecko/20081217 Firefox/2.0.0.20 everything is fine.

Comment 4

[(no longer active)]

(In reply to comment #2)
> The regression range is somewhere before FF3.0b3. Means back in 2007. The
> latest build I was able to test is 08010104. Builds from 2007 don't work with
> the force locale extension. I don't have time right now to get an exact time
> frame for rtl locales.

I wonder how much a regression window that old will be of help.  I think we can just fix it.  I bet it's a problem with the position of the popup (like the one we had in bug 427739 for example), but I'll be sure when I take a closer look.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

This should fix the problem.  I submitted a try server build with the rtlcustomize tag for Henrik to test.

Comment 6

[Phil Ringnalda (:philor)]

That really really feels like a bug in .openPopup - according to the screenshot, it's giving an x offset from the logical start of the element, and then treating "after_start" as a physical "after" == "right". Either one of those could be justified, but both together aren't very reasonable.

Regression range should be pretty easy to find, since you would probably only need to look at two things: when Mano landed the popup panel for Mac, and when Enn landed the popup rewrite.

Comment 7

[(no longer active)]

Try server build from my patch: <https://build.mozilla.org/tryserver-builds/2009-02-10_20:[e-mail]-rtlcustomize/>

Comment 8

[Neil Deakin]

The position "after_start" should already be handling right-to-left, assuming
that the popup has a computed style of 'direction: rtl'.

The x/y coordinates are not adjusted in this way though, but that has always
been the case and isn't a regression. I could see why that itself would be a
bug though.

Assuming that direction: rtl applies here, it would really be more correct to update the x/y rather than changing the position argument.

Comment 9

[(no longer active)]

Attachment: Patch (v2)

Comment 10

[(no longer active)]

Comment on attachment 361776 [details] [diff] [review]
Patch (v2)

Please disregard the previous patch and its try server build (I had forgot to qrefresh).  This new patch implements the behavior according to comment 8.  I'll also submit a try server build right away.

Comment 11

[Neil Deakin]

I don't follow this calculation:

+  if (rtl)
+    startPhysicalXOffset = sheetWidth - startPhysicalXOffset;

Can you explain what is trying to calculate?

Comment 12

[(no longer active)]

In LTR, the logical x offset is calculated from the left side, whereas for RTL it's calculated from the right side.  Since the offset is equal at both the left and right, |sheetWidth - startPhysicalXOffset| in RTL would be the logical x offset of the end (left-most) point of the popup, which is the physical x offset.

Comment 13

[(no longer active)]

Try server builds for the correct patch: <https://build.mozilla.org/tryserver-builds/2009-02-11_08:[e-mail]-rtlcustomize/>

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Even when the panel was rewritten for FF3.0 it is a regression against FF2.0.0.x. Adding back the keyword.

Ehsan, I ran your tryserver build with the force rtl extension, but the problem isn't fixed. Submitted the wrong patch?

Comment 15

[(no longer active)]

Can you please try it with the fa langpack so that we can be sure it's not a deficiency in Force RTL getting in the way?

BTW, just to check, you tested with the second try server build, right?  And did you observe anything different at all?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Showing with 2nd tryserver build

Yes, I did. See the attachment for the behavior with the fa.xpi installed instead of using the force locale extension.

Comment 17

[(no longer active)]

Ah, Neil, shouldn't this:

+    startPhysicalXOffset = sheetWidth - startPhysicalXOffset;

be:

+    startPhysicalXOffset = window.innerWidth - startPhysicalXOffset;

?  I think this is what I intended to do, but mistakenly used sheetWidth there.  I have submitted a new try server build.

Comment 18

[Neil Deakin]

That makes more sense, yes.

Comment 19

[(no longer active)]

Attachment: Patch (v3)

The patch with the fix mentioned in comment 17.

Comment 20

[Neil Deakin]

(In reply to comment #19)
> Created an attachment (id=361836) [details]
> Patch (v3)
> 
> The patch with the fix mentioned in comment 17.

So direction: rtl doesn't apply to the popup?

Comment 21

[(no longer active)]

(In reply to comment #20)
> So direction: rtl doesn't apply to the popup?

It was, that's why "after_start" position works, right?

Comment 22

[Neil Deakin]

In rtl mode, after_start should be aligning the popup along the right edge of
gNavToolbox then adding the x offset. Maybe I'm not interpreting this right,
but it seems to me that

if (rtl)
  startPhysicalXOffset = -startPhysicalXOffset;

is all that is needed.

Do you not have access to a Mac to test?

Comment 23

[Phil Ringnalda (:philor)]

You *should* be able to test the sheet's behavior without a Mac, by just removing the ifneq/endif around http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/Makefile.in#75 (though it's been a while since I last tried).

Comment 24

[(no longer active)]

Try server build available at: <https://build.mozilla.org/tryserver-builds/2009-02-11_12:[e-mail]-rtlcustomize3/>.  I don't have a Mac, but I'll try Phil's suggestion.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry but with the latest tryserver build its even getting worth. It's shifted more to the right as it was before.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #25)
> Sorry but with the latest tryserver build its even getting worth.

That should be called out worse! Hey, it as worth commenting on that.

Comment 27

[Neil Deakin]

Comment on attachment 361836 [details] [diff] [review]
Patch (v3)

Henrik implies that this isn't right.

Comment 28

[(no longer active)]

Attachment: Patch (v1, corrected) [landed on 1.9.1]

(In reply to comment #8)
> The position "after_start" should already be handling right-to-left, assuming
> that the popup has a computed style of 'direction: rtl'.

I think this may not be correct.  I don't see the source code handling RTL mode at all <http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuPopupFrame.cpp#451>.  In other words, both the position and x-offset arguments are physical, so I think position should be adjusted.  The reason my first patch didn't work was sending the wrong patch; I tested this one using the trick Phil suggested in comment 23.

Comment 29

[Neil Deakin]

It is handled in nsMenuPopupFrame::AdjustPositionForAnchorAlign.

Comment 30

[(no longer active)]

Attachment: Try to fix in core

Oh, I see.  Sorry for my mistake.

So, how would you like to fix this issue in core like this?

With this patch, the issue is solved, and I think all other popups work correctly.

Comment 31

[(no longer active)]

Attachment: Try to fix in core

Oops, wrong patch.

Comment 32

[Neil Deakin]

Comment on attachment 362103 [details] [diff] [review]
Try to fix in core

Could cause some compatibility problems, but this is probably the right thing to do. Maybe not for 3.1 though.

Care to write a simple test?

Comment 33

[(no longer active)]

(In reply to comment #32)
> (From update of attachment 362103 [details] [diff] [review])
> Could cause some compatibility problems, but this is probably the right thing
> to do. Maybe not for 3.1 though.

I agree.  Can we stick to attachment 362091 [details] [diff] [review] for 3.1?

> Care to write a simple test?

Sure, I'm on it.

Comment 34

[Neil Deakin]

> I agree.  Can we stick to attachment 362091 [details] [diff] [review] for 3.1?

OK, let's do that for 3.1 then.

Comment 35

[(no longer active)]

Comment on attachment 361836 [details] [diff] [review]
Patch (v3)

(for 3.1)

Comment 36

[Neil Deakin]

Comment on attachment 361836 [details] [diff] [review]
Patch (v3)

You want the 'v1, corrected' patch

Comment 37

[(no longer active)]

Attachment: Core fix

Patch for 1.9.2.  The code part is the same as attachment 362103 [details] [diff] [review].  I wrote a simple mochitest for this behavior as well.

Comment 38

[(no longer active)]

Comment on attachment 362091 [details] [diff] [review]
Patch (v1, corrected) [landed on 1.9.1]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dc200d20b109

Comment 39

[Philip Chee]

(In reply to Comment #23 From  Phil Ringnalda (:philor))
> You *should* be able to test the sheet's behavior without a Mac, by just
> removing the ifneq/endif around

In SeaMonkey you can just flip the pref |toolbar.customization.usesheet| to |true| no need to restart.

So attachment 362250 [details] [diff] [review] Isn't needed on 1.9.2 right? only 1.9.1?

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; fa; rv:1.9.1b3pre) Gecko/20090214 Shiretoko/3.1b3pre. Thanks Ehsan.

Comment 41

[(no longer active)]

(In reply to comment #39)
> So attachment 362250 [details] [diff] [review] Isn't needed on 1.9.2 right? only 1.9.1?


No, attachment 362091 [details] [diff] [review] is for 1.9.1 (which is landed and verified to fix this bug).  Attachment 362250 [details] [diff] fixes the core bug which causes this, and is intended for 1.9.2.

Comment 42

[Neil Deakin]

Comment on attachment 362250 [details] [diff] [review]
Core fix

This looks good, however the anchor label should appear closer to the middle of the window, for example:

<hbox pack="center">
  <label id="anchor" value="Anchor"/>
</hbox>

This way the popup constraining to the window size wouldn't have a possible impact on the outcome.
 
>+    let containerWidth = 300;
>+    let windowWidth = window.innerWidth;
>+    let popupWidth = 200;

These three aren't used.

Comment 43

[(no longer active)]

Landed with the requested changes: <http://hg.mozilla.org/mozilla-central/rev/2b893b177eca>

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; fa; rv:1.9.2a1pre) Gecko/20090210 Minefield/3.2a1pre ID:20090210022740