1415056 - [RTL] The Flexible Space button is moved outside of customization palette when dragging last button

Status: RESOLVED FIXED

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

Description

[magicp]

Steps to reproduce:
1. Launch Firefox in RTL locales (e.g. ar)
2. Open Customize...
3. Drag all of buttons to Overflow menu

Actual Results:
The Flexible Space button is moved outside of customization palette when dragging last button.

Expected Results:
Same behavior with LTR locales.

Comment 1

[magicp]

Attachment: bug1415056-str.mp4

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1415056 - re-enable animations in the palette in customize mode,

Review commit: https://reviewboard.mozilla.org/r/205090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205090/

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1415056 - reverse direction of horizontal shift in RTL when shifting palette items in customize mode,

This also fixes the vertical offset because we take the correct branch in the yDiff if block lower down.

Review commit: https://reviewboard.mozilla.org/r/205092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205092/

Comment 4

[:Gijs (he/him)]

Well, this took a long time, apologies for that. There's a lot of issues with dragging in the palette. I don't really want to fix all of them right now, because we're likely going to redo the palette significantly in bug 1388154. I'm only fixing the drag animations which were inadvertently broken by bug 1354082, as noted in bug 1395950, and this bug. Both of these are trivial fixes.

The drag animation fix (first cset) is because in this change:

https://hg.mozilla.org/mozilla-central/rev/119ce372e28e#l2.534

-      if (!aInToolbar) {
+      if (aAreaType == "palette" || (aAreaType == "menu-panel" && !gPhotonStructure)) {

we changed from basically checking "is this anywhere but in a toolbar" to "is this in the palette, or is this non-photon and is it in the panel".

Unfortunately, because CustomizableUI doesn't recognize the palette as an "area" (it only exists in customize mode), sometimes aAreaType was just null. This would have worked previously, but broke in the new version of the if statement.

Rather than adding an extra clause to the if statement to deal with the aAreaType being null, I figured it'd make more sense to make the aAreaType correct, so I fixed that instead.


This bug (second cset) is fairly self-explanatory -- once there are no more visible nodes to check for "where am I moving this thing" (which happens when there's only 2 things left in the palette, because we hide the 1 thing you drag out of the palette), we use the width of the item to displace it horizontally, but that is obviously always positive, when in RTL we should be moving the item in the opposite direction. This also fixes the vertical offset because the logic for it is already correct in checking for negative/positive horizontal offset for LTR/RTL to determine whether we also need to move the item one row vertically.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8934141 [details]
Bug 1415056 - re-enable animations in the palette in customize mode,

https://reviewboard.mozilla.org/r/205090/#review210778

::: browser/components/customizableui/CustomizeMode.jsm:1828
(Diff revision 1)
>      if (targetArea.id != kPaletteId &&
>          !CustomizableUI.canWidgetMoveToArea(draggedItemId, targetArea.id)) {
>        return;
>      }
>  
> -    let targetAreaType = CustomizableUI.getAreaType(targetArea.id);
> +    let targetAreaType = CustomizableUI.getAreaType(targetArea.id) || "palette";

Can we use getPlaceForItem? I feel like with the approach this patch is taking we're going to end up tacking on ` || "palette"` in a bunch of places.

https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/browser/components/customizableui/CustomizableUI.jsm#3878

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8934142 [details]
Bug 1415056 - reverse direction of horizontal shift in RTL when shifting palette items in customize mode,

https://reviewboard.mozilla.org/r/205092/#review210780

Comment 7

[:Gijs (he/him)]

Comment on attachment 8934141 [details]
Bug 1415056 - re-enable animations in the palette in customize mode,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205090/diff/1-2/

Comment 8

[:Gijs (he/him)]

Comment on attachment 8934142 [details]
Bug 1415056 - reverse direction of horizontal shift in RTL when shifting palette items in customize mode,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205092/diff/1-2/

Comment 9

[:Gijs (he/him)]

Comment on attachment 8934141 [details]
Bug 1415056 - re-enable animations in the palette in customize mode,

https://reviewboard.mozilla.org/r/205090/#review210782

::: browser/components/customizableui/CustomizeMode.jsm:1828
(Diff revision 1)
>      if (targetArea.id != kPaletteId &&
>          !CustomizableUI.canWidgetMoveToArea(draggedItemId, targetArea.id)) {
>        return;
>      }
>  
> -    let targetAreaType = CustomizableUI.getAreaType(targetArea.id);
> +    let targetAreaType = CustomizableUI.getAreaType(targetArea.id) || "palette";

Hm, I'd forgotten how that worked. Yeah, we can use that.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8934141 [details]
Bug 1415056 - re-enable animations in the palette in customize mode,

https://reviewboard.mozilla.org/r/205090/#review211850

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f0234c0ffb7
re-enable animations in the palette in customize mode, r=jaws
https://hg.mozilla.org/integration/autoland/rev/7ed8bedfc83c
reverse direction of horizontal shift in RTL when shifting palette items in customize mode, r=jaws

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8f0234c0ffb7
https://hg.mozilla.org/mozilla-central/rev/7ed8bedfc83c

Comment 13

[Sajedul Islam]

I have reproduced this bug with Nightly 58.0a1 (2017-11-06) (ar) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly (ar) 59.0a1 (2017-12-16) !

Build   ID    20171216100407
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:59.0) Gecko/20100101 Firefox/59.0

[bugday-20171213]