Closed Bug 1387182 Opened 2 years ago Closed 2 years ago

Toolbar icons "shaking" if holding an icon over another, while in customization mode on RTL builds

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: itiel_yn8, Assigned: Gijs)

References

Details

(Keywords: regression, Whiteboard: [photon-structure])

Attachments

(2 files)

Attached image Shaking icons
Using latest Nightly.
See attached demo.
Not sure if this is an intended behaviour.
Is this actually a regression? I kind of doubt it...
Flags: needinfo?(itiel_yn8)
(In reply to :Gijs from comment #1)
> Is this actually a regression? I kind of doubt it...

Well, I didn't think about whether this is a regression or not, but now that you've mentioned it, I just checked with mozregression and yes, it is a regression, though I don't think this is really the culprit:

2017-08-04T18:52:13: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=c0318c8fa15e76f6eaab2df21235b196f64187b2&full=1
2017-08-04T18:52:14: DEBUG : Found commit message:
Backed out changeset 502f442198b6 (bug 1360248) for build bustage in WasmSignalHandlers.cpp, at least on Windows 2012. r=backout on a CLOSED TREE

2017-08-04T18:52:14: INFO : The bisection is done.
2017-08-04T18:52:15: INFO : Stopped

The regression range is from 2017-05-10 to 2017-05-11, and happens only on RTL builds it seems.
Flags: needinfo?(itiel_yn8)
Has Regression Range: --- → yes
Keywords: regression
Summary: Toolbar icons "shaking" if holding an icon over another, while in customization mode → Toolbar icons "shaking" if holding an icon over another, while in customization mode on RTL builds
(In reply to ItielMaN from comment #2)
> The regression range is from 2017-05-10 to 2017-05-11, and happens only on
> RTL builds it seems.

Can this be reproduced on an LTR build when switching the direction of english to be RTL via the relevant about:config pref? The regression window doesn't look like it's correct...
Flags: needinfo?(itiel_yn8)
(In reply to :Gijs from comment #3)
> (In reply to ItielMaN from comment #2)
> > The regression range is from 2017-05-10 to 2017-05-11, and happens only on
> > RTL builds it seems.
> 
> Can this be reproduced on an LTR build when switching the direction of
> english to be RTL via the relevant about:config pref? The regression window
> doesn't look like it's correct...

Looks like yes...
Flags: needinfo?(itiel_yn8)
I broke this. :-(
Blocks: 1354082
Whiteboard: [photon-structure][triage]
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Flags: qe-verify+
Priority: -- → P1
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [photon-structure]
Version: unspecified → Trunk
Comment on attachment 8894470 [details]
Bug 1387182 - fix border (empty space) compensation when dragging items in customize mode in RTL,

https://reviewboard.mozilla.org/r/165646/#review170718

::: browser/components/customizableui/CustomizeMode.jsm:1714
(Diff revision 1)
>            let itemRect = this._dwu.getBoundsWithoutFlushing(dragOverItem);
>            let dropTargetCenter = itemRect.left + (itemRect.width / 2);
>            let existingDir = dragOverItem.getAttribute("dragover");
> +          let dirFactor = this._dir == "ltr" ? 1 : -1;
>            if (existingDir == "before") {
> -            dropTargetCenter += (parseInt(dragOverItem.style.borderInlineStartWidth) || 0) / 2;
> +            dropTargetCenter += (parseInt(dragOverItem.style.borderInlineStartWidth) || 0) / 2 * dirFactor;

The 'start' side border is on the right in RTL, so this means the center is further to the *left* which means the x coordinate should *decrease* by the border amount, not *increase*.

The inverse applies to the 'end' side border in the else block, of course.
Comment on attachment 8894470 [details]
Bug 1387182 - fix border (empty space) compensation when dragging items in customize mode in RTL,

https://reviewboard.mozilla.org/r/165646/#review170892
Attachment #8894470 - Flags: review?(jaws) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/845dd7955f8a
fix border (empty space) compensation when dragging items in customize mode in RTL, r=jaws
https://hg.mozilla.org/mozilla-central/rev/845dd7955f8a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Fixed on latest Nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Managed to reproduce the issue on an affected build (Firefox 57.0a1), using the steps to reproduce from Comment 0, on Windows 10 X 64 bit. 1387182 is fixed & verified.
build ID: 20170809100326
[bugday-20170809]
Comment on attachment 8894470 [details]
Bug 1387182 - fix border (empty space) compensation when dragging items in customize mode in RTL,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1354082
[User impact if declined]: broken drag/drop behaviour for RTL users
[Is this code covered by automated tests?]: yes, but they don't run in RTL which is why this wasn't caught
[Has the fix been verified in Nightly?]: yup
[Needs manual test from QE? If yes, steps to reproduce]: yeah,
1. use an RTL build or, in about:config, set intl.uidirection to 1 and restart
2. drag 1 button in the toolbar across other buttons in the toolbar

once you drag a button to both the left and right edge of another button (without dropping it in the middle) while the button is animating, this bug happens

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's tiny, well-understood, there's a lot of beta left, and the change will only affect RTL which is broken-ish anyway.

To be honest, before bug 1354082 the behaviour was also broken in RTL, just differently (we didn't make space on the correct side of the button making it awkward to dnd correctly). So with bug 1354082 and this bug fixed, the behaviour is actually reasonable now (yay!)
[String changes made/needed]: nope
Attachment #8894470 - Flags: approval-mozilla-beta?
Comment on attachment 8894470 [details]
Bug 1387182 - fix border (empty space) compensation when dragging items in customize mode in RTL,

Fix for odd behavior in customization menu for RTL, let's uplift this to beta.
Attachment #8894470 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.