963773 - The Toolbutton insertion position should consider amount of scroll of PanelUI

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/9d650c07b547
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140124030216

See screen capture: http://www.youtube.com/watch?v=Nwtn5BNDZ1A

Steps To Reproduce:
1. Make sure that PanelUI has scrollbar when customize mode
2. Enter Customize mode
3. Scroll down panel so that empty placeholder appears
4. Attempt to place a button on the empty placeholder

Actual Results:
The button is placed at wrong position

Expected Results:
The button should be placed at correct position

Comment 1

[:Gijs (he/him)]

Attachment: determine offset inside scrolling container for Australis customize mode drag placeholder UI to make things work when scrolling,

Well, that took a while. But this seems to work well. There's an issue where if there's only 1 placeholder or 1 element (I forget) at the end of the panel, and you drag between there and higher up (ie causing the number of rows to change while scrolling/dragging), the scrolling/dragging at the same time causes some issues, but that can be followup fodder if we think it's worth looking into.

Comment 2

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

Comment on attachment 8381748 [details] [diff] [review]
determine offset inside scrolling container for Australis customize mode drag placeholder UI to make things work when scrolling,

Review of attachment 8381748 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/src/DragPositionManager.jsm
@@ +76,5 @@
>     */
>    find: function(aContainer, aX, aY, aDraggedItemId) {
>      let closest = null;
>      let minCartesian = Number.MAX_VALUE;
> +    let containerX = this._containerInfo.left;

Whey do we need to change how we handle horizontal positioning? From my understanding of this bug, we should only need to change how we account for vertical offsets.

Comment 3

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Comment on attachment 8381748 [details] [diff] [review]
> determine offset inside scrolling container for Australis customize mode
> drag placeholder UI to make things work when scrolling,
> 
> Review of attachment 8381748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/DragPositionManager.jsm
> @@ +76,5 @@
> >     */
> >    find: function(aContainer, aX, aY, aDraggedItemId) {
> >      let closest = null;
> >      let minCartesian = Number.MAX_VALUE;
> > +    let containerX = this._containerInfo.left;
> 
> Whey do we need to change how we handle horizontal positioning? From my
> understanding of this bug, we should only need to change how we account for
> vertical offsets.

The only real reason was making it symmetrical, ie computing positioning in the same way for x and y. If you prefer me to only deal with the y bits, I could do that...

Comment 4

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

Comment on attachment 8381748 [details] [diff] [review]
determine offset inside scrolling container for Australis customize mode drag placeholder UI to make things work when scrolling,

Ok, you can leave it as-is as long as you are comfortable with our test coverage of this.

Comment 5

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Comment on attachment 8381748 [details] [diff] [review]
> determine offset inside scrolling container for Australis customize mode
> drag placeholder UI to make things work when scrolling,
> 
> Ok, you can leave it as-is as long as you are comfortable with our test
> coverage of this.

Well, comfortable... no, but not any less than I was already, and I don't see how to automatically test this properly considering the state of our drag simulation testing code.

remote:   https://hg.mozilla.org/integration/fx-team/rev/2e3cdc99a6ff

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2e3cdc99a6ff

Comment 7

[:Gijs (he/him)]

Comment on attachment 8381748 [details] [diff] [review]
determine offset inside scrolling container for Australis customize mode drag placeholder UI to make things work when scrolling,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis' customize mode dragging
User impact if declined: dragging widgets when the window is too small to contain the full panel (a real possibility on small-screen devices) doesn't work well
Testing completed (on m-c, etc.): local, on m-c
Risk to taking this patch (and alternatives if risky): reasonably low. The fix is quite contained, but unfortunately there are no automated tests for this feature because our drag simulation test framework code isn't very good. Only way to know is to try it and see if stuff still works correctly. I did my best on that front, but it's not as much of a guarantee as a proper automated test.
String or IDL/UUID changes made by this patch: none

Comment 8

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/475bc4af0a4b

Comment 9

[Ioana (away)]

I reproduced this issue on a Firefox 30.0 build from February 27th.

When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found some remaining issues:
1. When the last row of the panel contains only empty placeholders and you try to place a button on row X, row X+1 shift to make space for it. It's also placed on row X+1 if you actually drop the button on row X.

2. When the last row of the panel contains both empty placeholders and buttons, you can't place a new button on that last row anywhere but on the placeholder. When trying to place it before the first button on the row or between two existing buttons, the buttons move to make space for it, but when you drop it, it moves back to the Additional Tools and Features page.

Comment 10

[Ioana (away)]

(In reply to Ioana Budnar, QA [:ioana] from comment #9)
> I reproduced this issue on a Firefox 30.0 build from February 27th.
> 
> When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found
> some remaining issues:

These issues only reproduce when the window is slightly minimized, so the PanelUI has a scrollbar when in customize mode.

Comment 11

[:Gijs (he/him)]

(In reply to Ioana Budnar, QA [:ioana] from comment #9)
> I reproduced this issue on a Firefox 30.0 build from February 27th.
> 
> When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found
> some remaining issues:
> 1. When the last row of the panel contains only empty placeholders and you
> try to place a button on row X, row X+1 shift to make space for it. It's
> also placed on row X+1 if you actually drop the button on row X.

I'm not sure what you mean here. What is row "X"? Can you perhaps create a screencast of what you're seeing?

When having only empty placeholders in the panel and making the panel scrollable by resizing the window, when I drag and drop buttons from the toolbox to either the row of placeholders, or the row above it, or the row above that, the added buttons land where the space is created for them to land.

> 2. When the last row of the panel contains both empty placeholders and
> buttons, you can't place a new button on that last row anywhere but on the
> placeholder. When trying to place it before the first button on the row or
> between two existing buttons, the buttons move to make space for it, but
> when you drop it, it moves back to the Additional Tools and Features page.

I can reproduce this issue, and I'll file another bug for it (and possibly issue 1) when issue 1 is clear to me. I'm not really sure what causes this issue - if I try on Windows, I'm actually seeing the cursor change when dragging over the created empty space, indicating I can't drop there, and the "drop" handler for the drag drop code doesn't get called at all (instead, the drag gets 'cancelled' which is why the button returns to the toolbox). I don't yet understand why that is.

Comment 12

[Ioana (away)]

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Ioana Budnar, QA [:ioana] from comment #9)
> > I reproduced this issue on a Firefox 30.0 build from February 27th.
> > 
> > When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found
> > some remaining issues:
> > 1. When the last row of the panel contains only empty placeholders and you
> > try to place a button on row X, row X+1 shift to make space for it. It's
> > also placed on row X+1 if you actually drop the button on row X.
> 
> I'm not sure what you mean here. What is row "X"? Can you perhaps create a
> screencast of what you're seeing?

http://screencast.com/t/vcf6s5cf

Whilst yesterday I could reproduce this issue every time I tried, today it took many tries and a long while to get there, so I'd say this is quite highly intermittent.

Comment 13

[Mike Conley (:mconley) (:⚙️) (Catching up on needinfos)]

(In reply to Ioana Budnar, QA [:ioana] from comment #12)
> (In reply to :Gijs Kruitbosch from comment #11)
> > (In reply to Ioana Budnar, QA [:ioana] from comment #9)
> > > I reproduced this issue on a Firefox 30.0 build from February 27th.
> > > 
> > > When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found
> > > some remaining issues:
> > > 1. When the last row of the panel contains only empty placeholders and you
> > > try to place a button on row X, row X+1 shift to make space for it. It's
> > > also placed on row X+1 if you actually drop the button on row X.
> > 
> > I'm not sure what you mean here. What is row "X"? Can you perhaps create a
> > screencast of what you're seeing?
> 
> http://screencast.com/t/vcf6s5cf
> 
> Whilst yesterday I could reproduce this issue every time I tried, today it
> took many tries and a long while to get there, so I'd say this is quite
> highly intermittent.

It's quite possible this was made better by bug 992373.

Comment 14

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #13)
> (In reply to Ioana Budnar, QA [:ioana] from comment #12)
> > (In reply to :Gijs Kruitbosch from comment #11)
> > > (In reply to Ioana Budnar, QA [:ioana] from comment #9)
> > > > I reproduced this issue on a Firefox 30.0 build from February 27th.
> > > > 
> > > > When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found
> > > > some remaining issues:
> > > > 1. When the last row of the panel contains only empty placeholders and you
> > > > try to place a button on row X, row X+1 shift to make space for it. It's
> > > > also placed on row X+1 if you actually drop the button on row X.
> > > 
> > > I'm not sure what you mean here. What is row "X"? Can you perhaps create a
> > > screencast of what you're seeing?
> > 
> > http://screencast.com/t/vcf6s5cf
> > 
> > Whilst yesterday I could reproduce this issue every time I tried, today it
> > took many tries and a long while to get there, so I'd say this is quite
> > highly intermittent.
> 
> It's quite possible this was made better by bug 992373.

Yes, I hope so! However, it was intermittent before then... not sure what's going on there. I hope to find time today or otherwise tomorrow to investigate at least issue 2, and to see if I can still reproduce some form of issue 1. Setting needinfo so I don't forget...

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Mike Conley (:mconley) from comment #13)
> > (In reply to Ioana Budnar, QA [:ioana] from comment #12)
> > > (In reply to :Gijs Kruitbosch from comment #11)
> > > > (In reply to Ioana Budnar, QA [:ioana] from comment #9)
> > > > > I reproduced this issue on a Firefox 30.0 build from February 27th.
> > > > > 
> > > > > When trying to verify it on the latest Firefox 29, 30 and 31 builds, I found
> > > > > some remaining issues:
> > > > > 1. When the last row of the panel contains only empty placeholders and you
> > > > > try to place a button on row X, row X+1 shift to make space for it. It's
> > > > > also placed on row X+1 if you actually drop the button on row X.
> > > > 
> > > > I'm not sure what you mean here. What is row "X"? Can you perhaps create a
> > > > screencast of what you're seeing?
> > > 
> > > http://screencast.com/t/vcf6s5cf
> > > 
> > > Whilst yesterday I could reproduce this issue every time I tried, today it
> > > took many tries and a long while to get there, so I'd say this is quite
> > > highly intermittent.
> > 
> > It's quite possible this was made better by bug 992373.
> 
> Yes, I hope so! However, it was intermittent before then... not sure what's
> going on there. I hope to find time today or otherwise tomorrow to
> investigate at least issue 2, and to see if I can still reproduce some form
> of issue 1. Setting needinfo so I don't forget...

Apologies for the extreme delay here. I've had no luck reproducing issue 1, but I found the root cause of issue 2, for which I've filed bug 1004255.